Bug 80242 – REGRESSION(r106232): The resize handler is always called after loading

• Component: Platform
• Status: RESOLVED FIXED

[Kent Tamura]

Reported from a Google Chrome user: http://code.google.com/p/chromium/issues/detail?id=115963

After r106232, 'resize' event is always dispatched after a page loading if the page has a scrollbar. Other browsers don't have this behavior, and actual site is badly affected.

How to reproduce the problem:
Load the attached HTML, then you'll see 'The resize handler was called 1 times'.
Other browsers show 'The resize handler is NOT called.'

Another reproducible case:
Load http://sandisk-support.jp/ on Google Chrome 18 or later.  The site will be repeatedly reloaded.
Note that the site has no problem on Safari + WebKit nightly because the site changes behavior by UA detection.

[Kent Tamura]

Created attachment 130059 [details]
Reproducible HTML

[Allan Sandfeld Jensen]

I would say the resizeevent is logically correct since the viewport does resize, becoming smaller, when scrollbars are added. 

To be consistent with other browsers though I guess we need to only emit resize-events when the total size of layoutsize + all scrollbars changes. This should for the non-fixed viewport case give the same result as before r106232.

[Allan Sandfeld Jensen]

Created attachment 130363 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 130363 [details]
Patch

how will this work when the layout size changes but not the visible content rect?

[alan]

Comment on attachment 130363 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130363&action=review

> Source/WebCore/page/FrameView.cpp:228
> +    m_lastViewportSize = LayoutSize();

how about using IntSize() instead of LayoutSize() now that the variable has not much to do with layout size anymore? same in the .h

[alan]

(In reply to comment #4)
> (From update of attachment 130363 [details])
> how will this work when the layout size changes but not the visible content rect?
It looks to me like it works when zoomed at fit-to-width in content to resize mode.

[Allan Sandfeld Jensen]

Created attachment 130598 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 130598 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130598&action=review

> Source/WebCore/page/FrameView.cpp:1046
> +                    m_lastViewportSize = visibleContentRect(true).size();

/*includesScrollbars*/

Aer you sure that visibleContentRect returns the layout width in the case we (or someone else) are using fixed layout? Deserves a comment at least, or assert of something

[Allan Sandfeld Jensen]

Created attachment 130617 [details]
Patch

Additional comments

[Allan Sandfeld Jensen]

Created attachment 130618 [details]
Patch

Forgot to include ChangeLog diff.

[alan]

(In reply to comment #8)
> (From update of attachment 130598 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130598&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1046
> > +                    m_lastViewportSize = visibleContentRect(true).size();
> 
> /*includesScrollbars*/
> 
> Aer you sure that visibleContentRect returns the layout width in the case we (or someone else) are using fixed layout? Deserves a comment at least, or assert of something

this change looked confusing the first time, but after looking at it a little bit more, i think it is just fine.
in case of non-fixed layout, the only change is that the scrollbars are now included, while with fixed layout, it correctly uses the m_fixedVisibleContentRect instead of m_fixedLayoutSize.

[Kenneth Rohde Christiansen]

Comment on attachment 130618 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=130618&action=review

> Source/WebCore/page/FrameView.cpp:1046
> +                    m_lastViewportSize = visibleContentRect(true /*includeScrollbars*/).size();

This is not correct for the fixedlayout case:

 230 IntRect ScrollView::visibleContentRect(bool includeScrollbars) const
 231 {
 232     if (platformWidget())
 233         return platformVisibleContentRect(includeScrollbars);
 234 
 235     if (!m_fixedVisibleContentRect.isEmpty())
 236         return m_fixedVisibleContentRect;

Thus, this returns the visible content rect in CSS coordinates, and thus it will change every time the scale changes.

It also feels like you are abusing the method for something it was not designed for. We really want the layout sizes here and not what is visible (even if those might be the same in the desktop case)


Why not do LayoutSize(layoutWidth() + verticalScrollbar()->width(), layoutHeight() +  horizontalScrollbar()->height()); ?

> Source/WebCore/page/FrameView.cpp:2332
> +        // Resize events are not emitted when layoutsize changes due to added scrollbars, but are emitted if
> +        // the viewport is externally resized even if the resize does not affect layoutsize.

Due to compatibility with other browsers, we should not emit resize events when the layout size changes
due to scrollbars being added or removed; and thus only when the viewport is externally resized.

[Allan Sandfeld Jensen]

(In reply to comment #12)
> (From update of attachment 130618 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=130618&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1046
> > +                    m_lastViewportSize = visibleContentRect(true /*includeScrollbars*/).size();
> 
> This is not correct for the fixedlayout case:
> 
>  230 IntRect ScrollView::visibleContentRect(bool includeScrollbars) const
>  231 {
>  232     if (platformWidget())
>  233         return platformVisibleContentRect(includeScrollbars);
>  234 
>  235     if (!m_fixedVisibleContentRect.isEmpty())
>  236         return m_fixedVisibleContentRect;
> 
> Thus, this returns the visible content rect in CSS coordinates, and thus it will change every time the scale changes.
> 
That is actually intentional and matches what the browser in the N9 does. This means the resize-event is emitted when resizes in the UIProcess happens, even if they do not change the layout-size, because they do change the size of the visible rect. Whether or not it is the "right thing" to do is a good question, but it is a not mistake.

[Allan Sandfeld Jensen]

Created attachment 130800 [details]
Patch

[Allan Sandfeld Jensen]

The new patch takes a more conservative approach and only uses fixed-layout-size. The new size has been encapsulated in a new function which should make it more clear what happens, and means we can change what size we use later by just modifying that function.

[Allan Sandfeld Jensen]

Created attachment 130802 [details]
Patch

Wrong baseline for test.

[Kenneth Rohde Christiansen]

Comment on attachment 130802 [details]
Patch

I am getting to feel that this is more complicated as such. Basically the spec says:

The resize event occurs when a document view is resized. (http://www.w3.org/TR/DOM-Level-2-Events/events.html)

I believe that Chrome for Android also uses fixed layout but I am not sure how they resize their FrameView.

Maybe we should actually just use the width() and height() { return frameRect().height() }. and instead check for "delegatesScrolling()" 

I guess the question is, when is the resize event supposed to be emitted on a mobile browser? during load of new page? after orientation change? after restore from page cache? I think we need to define this or test it on other platforms and then create tests for that.

[Lars Knudsen]

Not too far into the future, we might also be interested in being able to swap between different screen outputs (e.g. when you get home, you might want to have your TV render the web page you are currently looking at on your mobile device), resulting in an actual resizing.  Regarding the issue with scroll bars:  on touch devices, scroll indicators make sense - not bars.  Scroll indicators should not resize the view.  On desktop/non touch/mouse versions of the same, one could argue that scroll bars are also possible to replace with scroll indicators that appear/grow when hovering in the region of interest (which is more or less what my Ubuntu seems to do now anyway).  Again - this (IMHO) would be a smarter approach than resizing everything because of legacy controls (my grandmother used to use...).

(In reply to comment #17)
> (From update of attachment 130802 [details])
> I am getting to feel that this is more complicated as such. Basically the spec says:
> 
> The resize event occurs when a document view is resized. (http://www.w3.org/TR/DOM-Level-2-Events/events.html)
> 
> I believe that Chrome for Android also uses fixed layout but I am not sure how they resize their FrameView.
> 
> Maybe we should actually just use the width() and height() { return frameRect().height() }. and instead check for "delegatesScrolling()" 
> 
> I guess the question is, when is the resize event supposed to be emitted on a mobile browser? during load of new page? after orientation change? after restore from page cache? I think we need to define this or test it on other platforms and then create tests for that.

[alan]

After a quick search on the subject, it seems mobile browsers behave very differently when it comes to reporting resize event. Here is a good summary http://www.quirksmode.org/dom/events/resize_mobile.html

[Allan Sandfeld Jensen]

(In reply to comment #19)
> After a quick search on the subject, it seems mobile browsers behave very differently when it comes to reporting resize event. Here is a good summary http://www.quirksmode.org/dom/events/resize_mobile.html

That looks interesting. The patch that compares visual-rect-size would act like Opera Mobile, Android and Blackberry.

The most recent patch that only compares fixed-layout-size would work only fire when the fixed layout-size changes. Essentially only on viewport configuration changes (new output screen, accessibility and probably orientation-change).

I would still go with the first of the two options, but given the inconsistency, you could argue for both.

[Kenneth Rohde Christiansen]

(In reply to comment #20)
> (In reply to comment #19)
> > After a quick search on the subject, it seems mobile browsers behave very differently when it comes to reporting resize event. Here is a good summary http://www.quirksmode.org/dom/events/resize_mobile.html
> 
> That looks interesting. The patch that compares visual-rect-size would act like Opera Mobile, Android and Blackberry.
> 
> The most recent patch that only compares fixed-layout-size would work only fire when the fixed layout-size changes. Essentially only on viewport configuration changes (new output screen, accessibility and probably orientation-change).
> 
> I would still go with the first of the two options, but given the inconsistency, you could argue for both.

I don't understand what you are arguing for here :-) but I personally think that only emitting the resize on changed fixed layout makes the most sense. It would be good to test whether the iphone actually does that or just always emits it on orientation change.

[Allan Sandfeld Jensen]

(In reply to comment #21)
> I don't understand what you are arguing for here :-) but I personally think that only emitting the resize on changed fixed layout makes the most sense. It would be good to test whether the iphone actually does that or just always emits it on orientation change.

How would you do that? The fixed layout size, is kind of fixed. The only time it would make sense to change is when changing media, and even then it is not guaranteed to change, and I am not sure the iPhone supports anything like that.

Anyway, I think we should commit one of the two patches before continuing the debate. This is a regression bug, and both options solve both the regression and the original bug. What we are discussing now is a different issue.

[Kenneth Rohde Christiansen]

Every time I read this code I have to remember what it is supposed to do.

I just find something like LayoutSize(layoutWidth() + verticalScrollbar()->width(), layoutHeight() +  horizontalScrollbar()->height()); so much more clear

It is to the point, and makes it very obvious that we are not emitting when scrollbars are added/removed. And with a comment on top stating that, this would be very easily understandable.

I really would like to avoid adding a widgetSize which is different that frameRect().size() - this just makes everything more confusing.

So first of all, how does this work in iOS/Android etc.

Are they sending the event when the actual widget changes size? or when the layout size changes? You can create a page that doesn't change when rotating and check whether the event gets emitted, and another one changing the layout height.

[Allan Sandfeld Jensen]

Created attachment 131854 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 131854 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=131854&action=review

> Source/WebCore/page/FrameView.cpp:2310
> +        // DOM Level 2 Events: The resize event occurs when a document view is resized.
> +        IntSize currentSize = logicalFrameRect().size();

So the frameRect includes the scrollbars and we are the only ones having the issue of emitting events in the wrong place as we are using resize to contents mode which changes the frame rect a lot. Chrome for Android does use fixed layout but I don't think they use the resize to content move, so this might be wrong for them.

I guess we should do:

// DOM Level 2 Events: The resize event occurs when a document view is resized.
// In the case of delegated viewport scrolling, the frame rect is resized together
// with the content, so in that case we only send the even  when the layout size changed.
IntSize currentSize = delegatesScrolling() ? m_fixedLayoutSize : frameRect().size();

> Source/WebCore/platform/ScrollView.h:162
> +    // Logical frame rect is by default the same as the Widget::frameRect, but if fixedLayoutSize it used, it is instead
> +    // the rect of a virtual frame around the fixed layout area.
> +    IntRect logicalFrameRect() const;

I didn't think frameRect contained the scrollbars (sorry) so then maybe this is not the right solution :-(

[Allan Sandfeld Jensen]

(In reply to comment #25)
> (From update of attachment 131854 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=131854&action=review
> 
> > Source/WebCore/page/FrameView.cpp:2310
> > +        // DOM Level 2 Events: The resize event occurs when a document view is resized.
> > +        IntSize currentSize = logicalFrameRect().size();
> 
> So the frameRect includes the scrollbars and we are the only ones having the issue of emitting events in the wrong place as we are using resize to contents mode which changes the frame rect a lot. Chrome for Android does use fixed layout but I don't think they use the resize to content move, so this might be wrong for them.
> 
In the Qt case the frame-rect changes when the document size changes. It was not resized when the viewport was resized. It was resized by adding content to the page. This is why we can't use it, because in our case the frame-rect does not represent the document view, but the document itself.

That leaves the question: Is the document view, the viewport (visible rect), or the virtual layout area (fixed layout).

In either case. It is just a matter of semantics. It is pretty much the same for all mobile browsers, which is why we have the hack in iOS and N9 to avoid resize-events during page-parsing.

[WebKit Review Bot]

Attachment 131854 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
Source/WebCore/platform/ScrollView.cpp:257:  Missing space after ,  [whitespace/comma] [3]
Total errors found: 1 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Kenneth Rohde Christiansen]

(In reply to comment #26)
> (In reply to comment #25)
> > (From update of attachment 131854 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=131854&action=review
> > 
> > > Source/WebCore/page/FrameView.cpp:2310
> > > +        // DOM Level 2 Events: The resize event occurs when a document view is resized.
> > > +        IntSize currentSize = logicalFrameRect().size();
> > 
> > So the frameRect includes the scrollbars and we are the only ones having the issue of emitting events in the wrong place as we are using resize to contents mode which changes the frame rect a lot. Chrome for Android does use fixed layout but I don't think they use the resize to content move, so this might be wrong for them.
> > 
> In the Qt case the frame-rect changes when the document size changes. It was not resized when the viewport was resized. It was resized by adding content to the page. This is why we can't use it, because in our case the frame-rect does not represent the document view, but the document itself.
> 
> That leaves the question: Is the document view, the viewport (visible rect), or the virtual layout area (fixed layout).

So let's divide it into two groups

Desktop (non-delegated scrolling): 
   document view
   which in non-scaled situations is equal to the visible rect incl. scrollbars
   which is basically equal to the layout area + scrollbars
   

Mobile (delegated scrolling):
   layout area (incl scrollbars which are overlays and thus occupy 0 size)
   which when content is laid out and scaled to fit is pretty much (forget viewport meta initial-scale) the document view



> In either case. It is just a matter of semantics. It is pretty much the same for all mobile browsers, which is why we have the hack in iOS and N9 to avoid resize-events during page-parsing.

Sure, that could be done as well if we do delegated scrolling. Delegated scrolling is not the best word, but I have no better suggestion.


// The spec DOM Level 2 Events states that the resize event occurs when a document view is resized.
// What is actually the document view is a bit ambiguous considering mobile browsers. The best term
// that describes the current behavior is: the initial non-user-scaled view (incl. non-overlay scrollbars).

IntSize currentSize;

if (!delegatesScrolling()) {
    // Desktop (non-delegated scrolling)
    currentSize = visibleRect(/*includeScrollbars*/ true).size();
} else {
    // Mobile (delegated scrolling).
    currentSize = LayoutSize(layoutWidth() + verticalScrollbar()->width(), layoutHeight() +  horizontalScrollbar()->height());
}


I am including scrollbars below as it is possible to draw overlay scrollbars not overlaying the content, by twisting the RenderTheme to reserve the space.

As both are basically doing the same, I think we could just do:

IntSize currentSize = LayoutSize(layoutWidth() + verticalScrollbar()->width(), layoutHeight() +  horizontalScrollbar()->height());


I just tested on OS X Lion with Christian and it seems that safari there doesn't even use this code for resize (if you click the maximize button it doesn't get a resize event!). It also doesn't get resize events when user scaling, which should happen if visibleRect was used.

[Allan Sandfeld Jensen]

If you want to do the patch yourself, fine. Then I will review it.

(In reply to comment #28)
> // The spec DOM Level 2 Events states that the resize event occurs when a document view is resized.
> // What is actually the document view is a bit ambiguous considering mobile browsers. The best term
> // that describes the current behavior is: the initial non-user-scaled view (incl. non-overlay scrollbars).
> 
> IntSize currentSize;
> 
> if (!delegatesScrolling()) {
>     // Desktop (non-delegated scrolling)
>     currentSize = visibleRect(/*includeScrollbars*/ true).size();
> } else {
>     // Mobile (delegated scrolling).
>     currentSize = LayoutSize(layoutWidth() + verticalScrollbar()->width(), layoutHeight() +  horizontalScrollbar()->height());
> }
> 
DelagatesScrolling while in the Qt case is set together with fixedLayout, is a completely different setting that has nothing to do with fixedLayoutSize. Please check if fixedLayoutSize is set instead, see ScrollView::layoutWidth how it is done.

You are calculating scrollbar width wrong. You DO need to account for overlay or not, see ScrollView::visibleRect() for how it is done.

[Allan Sandfeld Jensen]

Created attachment 132015 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 132015 [details]
Patch

Missing the test, but r=me with the test

[Kenneth Rohde Christiansen]

Comment on attachment 132015 [details]
Patch

You are missing the test, which you refer to in the changelog. Please create a new patch including the test and results

[Allan Sandfeld Jensen]

Created attachment 132025 [details]
Patch for landing

Readd the regression test.

[WebKit Review Bot]

Comment on attachment 132025 [details]
Patch for landing

Clearing flags on attachment: 132025

Committed r110938: <http://trac.webkit.org/changeset/110938>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.