Created attachment 130059 [details] Reproducible HTML

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.

Created attachment 130363 [details] Patch

Comment on attachment 130363 [details] Patch how will this work when the layout size changes but not the visible content rect?

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

(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.

Created attachment 130598 [details] Patch

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

Created attachment 130617 [details] Patch Additional comments

Created attachment 130618 [details] Patch Forgot to include ChangeLog diff.

(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.

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.

(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.

Created attachment 130800 [details] Patch

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.

Created attachment 130802 [details] Patch Wrong baseline for test.

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.

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.

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

(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.

(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.

(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.

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.

Created attachment 131854 [details] Patch

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 :-(

(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.

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.

(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.

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.

Created attachment 132015 [details] Patch

Comment on attachment 132015 [details] Patch Missing the test, but r=me with the test

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

Created attachment 132025 [details] Patch for landing Readd the regression test.

Comment on attachment 132025 [details] Patch for landing Clearing flags on attachment: 132025 Committed r110938: <http://trac.webkit.org/changeset/110938>

All reviewed patches have been landed. Closing bug.