Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
22661007:
Make the body not scrollable and make a scrollable container div to host #extension-settings. (Closed)
|
Created:
7 years, 4 months ago by zhchbin Modified:
7 years, 4 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, arv+watch_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionMake the body not scrollable and make a scrollable
container div to host #extension-settings.
BUG=269450
TEST=1. Open "chrome://extensions" and add numbers of extensions;
2. Click on the "keyboard Shotcuts" link at the bottom of the page;
3. Scroll down and up the "keyboard shotcuts" page using mouse, now the page
should't scroll down and up.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=217952
Patch Set 1 #Patch Set 2 : Move to higher level of overlay. #
Total comments: 2
Patch Set 3 : Revise #Patch Set 4 : Revise as xiyuan's instruction. #
Total comments: 4
Patch Set 5 : Fix missing end closing tag. #
Messages
Total messages: 25 (0 generated)
Please review.
LGTM, but before committing, will you please take a minute to think whether there's a more general solution that will apply to all these situations? It's possible that there is only one scrolling page with an overlay, but if there are more than one, can we fix them all in one place?
On 2013/08/12 21:59:55, miket wrote: > LGTM, but before committing, will you please take a minute to think whether > there's a more general solution that will apply to all these situations? It's > possible that there is only one scrolling page with an overlay, but if there are > more than one, can we fix them all in one place? Thanks miket. PTAL again, is here a right way to go?
On 2013/08/13 09:48:19, zhchbin wrote: > On 2013/08/12 21:59:55, miket wrote: > > LGTM, but before committing, will you please take a minute to think whether > > there's a more general solution that will apply to all these situations? It's > > possible that there is only one scrolling page with an overlay, but if there > are > > more than one, can we fix them all in one place? > > Thanks miket. PTAL again, is here a right way to go? Excellent. I think this looks much, much better, don't you? LGTM and thanks!
On 2013/08/13 15:59:46, miket wrote: > On 2013/08/13 09:48:19, zhchbin wrote: > > On 2013/08/12 21:59:55, miket wrote: > > > LGTM, but before committing, will you please take a minute to think whether > > > there's a more general solution that will apply to all these situations? > It's > > > possible that there is only one scrolling page with an overlay, but if there > > are > > > more than one, can we fix them all in one place? > > > > Thanks miket. PTAL again, is here a right way to go? > > Excellent. I think this looks much, much better, don't you? LGTM and thanks! Thanks for your instruction and review!!
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/22661007/5001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
ping @xiyuan for owner review.
ping @xiyuan for owner review.
The "no-scroll" css would cause the scroll bar to hidden thus triggers a page layout. I would rather to keep the page scrollable than seeing it jump. Is there a way that we could avoid this? https://codereview.chromium.org/22661007/diff/5001/ui/webui/resources/js/cr/u... File ui/webui/resources/js/cr/ui/overlay.js (right): https://codereview.chromium.org/22661007/diff/5001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/overlay.js:94: function onVisibilityChanged(visible) { |visible| is mis-leading. It looks more like hidden.
>The "no-scroll" css would cause the scroll bar to hidden thus triggers a page layout. I would rather to keep the page scrollable than seeing it jump. Is there a way that we could avoid this? I haven't found a better way without hiding scroll bar. And I agree that we should avoid the page layout if we can. https://codereview.chromium.org/22661007/diff/5001/ui/webui/resources/js/cr/u... File ui/webui/resources/js/cr/ui/overlay.js (right): https://codereview.chromium.org/22661007/diff/5001/ui/webui/resources/js/cr/u... ui/webui/resources/js/cr/ui/overlay.js:94: function onVisibilityChanged(visible) { On 2013/08/13 17:23:35, xiyuan wrote: > |visible| is mis-leading. It looks more like hidden. Done.
If all we care here is the mouse wheel scroll (i.e. keyboard scroll and dragging
scrollbar is acceptable), you can add a "mousewheel" event handler to overlay to
ignore it.
Something like the following (you need to do it in the proper way in the CL):
$('overlay').addEventListener('mousewheel', function(e){e.preventDefault();});
On 2013/08/14 16:34:29, xiyuan wrote:
> If all we care here is the mouse wheel scroll (i.e. keyboard scroll and
dragging
> scrollbar is acceptable), you can add a "mousewheel" event handler to overlay
to
> ignore it.
>
> Something like the following (you need to do it in the proper way in the CL):
> $('overlay').addEventListener('mousewheel', function(e){e.preventDefault();});
Thanks for the instruction.
However, if the main content of overlay dialog is scrollable, this will disable
the mouse wheel scroll of it also. Haven't dive into yet, just my first thought.
By the way, I also find that the same problem didn't exist on chrome://settings
page. And the scroll bar will be hidden.
On 2013/08/15 02:21:58, zhchbin wrote:
> On 2013/08/14 16:34:29, xiyuan wrote:
> > If all we care here is the mouse wheel scroll (i.e. keyboard scroll and
> dragging
> > scrollbar is acceptable), you can add a "mousewheel" event handler to
overlay
> to
> > ignore it.
> >
> > Something like the following (you need to do it in the proper way in the
CL):
> > $('overlay').addEventListener('mousewheel',
function(e){e.preventDefault();});
>
> Thanks for the instruction.
>
> However, if the main content of overlay dialog is scrollable, this will
disable
> the mouse wheel scroll of it also. Haven't dive into yet, just my first
thought.
> By the way, I also find that the same problem didn't exist on
chrome://settings
> page. And the scroll bar will be hidden.
On chrome:extensions, #overlay is the container. The overlay dialog divs should
still be mouse wheel scrollable when we ignore mousewheel event on #overlay.
On chrome:options, check out OptionsPage.setRootPageFrozen_. When it freezes the
page, it sets the width etc and the page does not jump.
> On chrome:extensions, #overlay is the container. The overlay dialog divs should > still be mouse wheel scrollable when we ignore mousewheel event on #overlay. Yes, it should be, but if we handle the mousewheel event using preventDefault(), it will disable that. Maybe there will be some tricky ways to make it work again. > > On chrome:options, check out OptionsPage.setRootPageFrozen_. When it freezes the > page, it sets the width etc and the page does not jump. To prevent chrome:options page from jumping, it sets the position of page-container to fixed. This will also hide the scroll bar and cause a page layout. And it need to set and unset some attribution of "style" too when showing and hiding the overlay dialog. By the way, I don't think keyboard scroll and dragging scroll bar are acceptable.
On 2013/08/15 03:02:00, zhchbin wrote: > > On chrome:extensions, #overlay is the container. The overlay dialog divs > should > > still be mouse wheel scrollable when we ignore mousewheel event on #overlay. > > Yes, it should be, but if we handle the mousewheel event using preventDefault(), > it will disable that. Maybe there will be some tricky ways to make it work > again. > > > > > On chrome:options, check out OptionsPage.setRootPageFrozen_. When it freezes > the > > page, it sets the width etc and the page does not jump. > > To prevent chrome:options page from jumping, it sets the position of > page-container to fixed. This will also hide the scroll bar and cause a page > layout. And it need to set and unset some attribution of "style" too when > showing and hiding the overlay dialog. > > By the way, I don't think keyboard scroll and dragging scroll bar are > acceptable. If we need to handle keyboard scroll and scrollbar-thumb-dragging scroll, options page seems doing the right thing. That is, setting a proper width when disabling the scrollbar to avoid the relayout.
On 2013/08/15 04:14:36, xiyuan wrote: > On 2013/08/15 03:02:00, zhchbin wrote: > > > On chrome:extensions, #overlay is the container. The overlay dialog divs > > should > > > still be mouse wheel scrollable when we ignore mousewheel event on #overlay. > > > > Yes, it should be, but if we handle the mousewheel event using > preventDefault(), > > it will disable that. Maybe there will be some tricky ways to make it work > > again. > > > > > > > > On chrome:options, check out OptionsPage.setRootPageFrozen_. When it freezes > > the > > > page, it sets the width etc and the page does not jump. > > > > To prevent chrome:options page from jumping, it sets the position of > > page-container to fixed. This will also hide the scroll bar and cause a page > > layout. And it need to set and unset some attribution of "style" too when > > showing and hiding the overlay dialog. > > > > By the way, I don't think keyboard scroll and dragging scroll bar are > > acceptable. > > If we need to handle keyboard scroll and scrollbar-thumb-dragging scroll, > options page seems doing the right thing. That is, setting a proper width when > disabling the scrollbar to avoid the relayout. Setting a proper width when disabling the scroll bar can avoid the relayout? I do some simple experiments and debug with Speed Tracer, it seems will perform layout also. However, I have little knowledge about browser's rendering engine and this is just my personal idea about this problem.
On 2013/08/15 06:06:47, zhchbin wrote:
> On 2013/08/15 04:14:36, xiyuan wrote:
> > On 2013/08/15 03:02:00, zhchbin wrote:
> > > > On chrome:extensions, #overlay is the container. The overlay dialog divs
> > > should
> > > > still be mouse wheel scrollable when we ignore mousewheel event on
> #overlay.
> > >
> > > Yes, it should be, but if we handle the mousewheel event using
> > preventDefault(),
> > > it will disable that. Maybe there will be some tricky ways to make it work
> > > again.
> > >
> > > >
> > > > On chrome:options, check out OptionsPage.setRootPageFrozen_. When it
> freezes
> > > the
> > > > page, it sets the width etc and the page does not jump.
> > >
> > > To prevent chrome:options page from jumping, it sets the position of
> > > page-container to fixed. This will also hide the scroll bar and cause a
page
> > > layout. And it need to set and unset some attribution of "style" too when
> > > showing and hiding the overlay dialog.
> > >
> > > By the way, I don't think keyboard scroll and dragging scroll bar are
> > > acceptable.
> >
> > If we need to handle keyboard scroll and scrollbar-thumb-dragging scroll,
> > options page seems doing the right thing. That is, setting a proper width
when
> > disabling the scrollbar to avoid the relayout.
>
> Setting a proper width when disabling the scroll bar can avoid the relayout? I
> do some simple experiments and debug with Speed Tracer, it seems will perform
> layout also. However, I have little knowledge about browser's rendering engine
> and this is just my personal idea about this problem.
Sorry for not being clear. The layout would happen when the style changes. But
if we get the proper width/left/top etc set correct, it should produce the same
layout/visuals. To the end user, it would appear the page does not change when
the scrollbar disappears/shows. The problem is more apparent when the browser
window is not wide enough.
After thinking more on this, the problem here is the scroll event bubbles all
the way up to body and body is scrollable regardless of whether the overlay is
shown or not. Maybe we could make the body not scrollable and make a scrollable
container div to host #extension-settings. This way, #extension-settings is
still scrollable but the scroll event on overlay would only hit a non-scrollable
body.
Basically, here is what I think it might work.
- Add a <div id="page-container"> to wrap #extension-settings
- In CSS, let's make
html, body {
height: 100%;
overflow: hidden;
}
#page-container {
overflow-y: auto;
height: 100%;
}
If this solves the problem, then we probably should extend and apply the trick
to options page too. It would simple the logic quite a bit.
On 2013/08/15 08:43:55, xiyuan wrote:
> On 2013/08/15 06:06:47, zhchbin wrote:
> > On 2013/08/15 04:14:36, xiyuan wrote:
> > > On 2013/08/15 03:02:00, zhchbin wrote:
> > > > > On chrome:extensions, #overlay is the container. The overlay dialog
divs
> > > > should
> > > > > still be mouse wheel scrollable when we ignore mousewheel event on
> > #overlay.
> > > >
> > > > Yes, it should be, but if we handle the mousewheel event using
> > > preventDefault(),
> > > > it will disable that. Maybe there will be some tricky ways to make it
work
> > > > again.
> > > >
> > > > >
> > > > > On chrome:options, check out OptionsPage.setRootPageFrozen_. When it
> > freezes
> > > > the
> > > > > page, it sets the width etc and the page does not jump.
> > > >
> > > > To prevent chrome:options page from jumping, it sets the position of
> > > > page-container to fixed. This will also hide the scroll bar and cause a
> page
> > > > layout. And it need to set and unset some attribution of "style" too
when
> > > > showing and hiding the overlay dialog.
> > > >
> > > > By the way, I don't think keyboard scroll and dragging scroll bar are
> > > > acceptable.
> > >
> > > If we need to handle keyboard scroll and scrollbar-thumb-dragging scroll,
> > > options page seems doing the right thing. That is, setting a proper width
> when
> > > disabling the scrollbar to avoid the relayout.
> >
> > Setting a proper width when disabling the scroll bar can avoid the relayout?
I
> > do some simple experiments and debug with Speed Tracer, it seems will
perform
> > layout also. However, I have little knowledge about browser's rendering
engine
> > and this is just my personal idea about this problem.
>
> Sorry for not being clear. The layout would happen when the style changes. But
> if we get the proper width/left/top etc set correct, it should produce the
same
> layout/visuals. To the end user, it would appear the page does not change when
> the scrollbar disappears/shows. The problem is more apparent when the browser
> window is not wide enough.
>
> After thinking more on this, the problem here is the scroll event bubbles all
> the way up to body and body is scrollable regardless of whether the overlay is
> shown or not. Maybe we could make the body not scrollable and make a
scrollable
> container div to host #extension-settings. This way, #extension-settings is
> still scrollable but the scroll event on overlay would only hit a
non-scrollable
> body.
>
> Basically, here is what I think it might work.
> - Add a <div id="page-container"> to wrap #extension-settings
> - In CSS, let's make
> html, body {
> height: 100%;
> overflow: hidden;
> }
> #page-container {
> overflow-y: auto;
> height: 100%;
> }
>
> If this solves the problem, then we probably should extend and apply the trick
> to options page too. It would simple the logic quite a bit.
Thanks very much for @xiyuan's detailed clarification and instruction. I have
update this CL following your instruction.
Ping @miket for owner review again.
And if this is acceptable, I would like to follow up to revise chrome://settings
and chrome://history etc via another CL.
lgtm https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... chrome/browser/resources/extensions/extensions.html:63: <input id="toggle-dev-on" type="checkbox"> I think this needs a closing tag https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... chrome/browser/resources/extensions/extensions.html:64: <span i18n-content="extensionSettingsDeveloperMode"> missing </span> (please check the rest and make sure the markup validates)
lgtm lgtm https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... chrome/browser/resources/extensions/extensions.html:63: <input id="toggle-dev-on" type="checkbox"> I think this needs a closing tag https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... chrome/browser/resources/extensions/extensions.html:64: <span i18n-content="extensionSettingsDeveloperMode"> missing </span> (please check the rest and make sure the markup validates)
LGTM Thanks for the change. Please help to check the HTML as Mike pointed out. https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... chrome/browser/resources/extensions/extensions.html:63: <input id="toggle-dev-on" type="checkbox"> On 2013/08/15 15:36:20, miket wrote: > I think this needs a closing tag This should be fine in HTML. It only needs to be closed for XHTML.
Thanks very much for the review. https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... File chrome/browser/resources/extensions/extensions.html (right): https://codereview.chromium.org/22661007/diff/28001/chrome/browser/resources/... chrome/browser/resources/extensions/extensions.html:64: <span i18n-content="extensionSettingsDeveloperMode"> On 2013/08/15 15:36:20, miket wrote: > missing </span> > > (please check the rest and make sure the markup validates) Done. I have validated it with W3C Markup Validation Service. (I just made indented these line, seems like a problem left over by history.)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/zhchbin@gmail.com/22661007/35001
Message was sent while issue was closed.
Change committed as 217952 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
