Bug 108205

Created attachment 185266 [details] Patch

will probably need to add an extra test for orientation-change (i.e. size change)

Created attachment 188118 [details] Patch

Created attachment 188593 [details] Patch

in the last patch we decided to go with a simpler approach than in the previous two patches. Empirical tests suggest the approach of fixing the autosizing multiplier once it is set works sufficiently well for most webpages and in addition to preventing oscillation also results in faster layout times.

Comment on attachment 188593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188593&action=review Added some comments. > Source/WebCore/ChangeLog:8 > + On some websites font sizes appear to oscillate or grow very large due to You can delete "appear to", since they really do oscillate. And it's not really a case of them growing very large, it's more that on some sites it takes many layouts before the font sizes settle to a steady size. > Source/WebCore/page/Settings.cpp:369 > + // TODO: I wonder if this needs to traverse frames like in WebViewImpl::resize, or whether there is only one document per Settings instance? s/TODO/FIXME/ according to WebKit style. > Source/WebCore/rendering/TextAutosizer.cpp:99 > + setMultiplier(renderer, 1); Please add a check for renderer->style()->textAutosizingMultiplier() != 1, as setting the multiplier unnecessarily is quite expensive. > Source/WebCore/rendering/TextAutosizer.cpp:199 > + if (localMultiplier != descendant->style()->textAutosizingMultiplier() Nit: might be simpler/cheaper to write: localMultiplier != 1 && descendant->style()->textAutosizingMultiplier() == 1 > LayoutTests/ChangeLog:8 > + Changed old tests to layout without the scrollbar initially, because the A little unclear. Perhaps change this sentence to "Added overflow-y:hidden to some existing tests, since previously those tests would start off with incorrect multipliers (because mainFrame->view()->layoutSize() is initially 785 instead of 800 as ScrollView wrongly guesses a scrollbar will be needed), and then the multipliers would get corrected on a subsequent layout. Now that we don't allow the multiplier to change after being set, it needs to be right first time." > LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:42 > +if(element.offsetHeight){ Nit: space before ( and { please (ditto below). > LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:43 > + // force layout (to computation of offsetHeight triggers reflow) Nit: s/to // > LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:45 > +element.className='largersize'; Nit: spaces around = please (ditto below).

Comment on attachment 188593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188593&action=review >> Source/WebCore/ChangeLog:8 >> + On some websites font sizes appear to oscillate or grow very large due to > > You can delete "appear to", since they really do oscillate. > And it's not really a case of them growing very large, it's more that on some sites it takes many layouts before the font sizes settle to a steady size. On some websites autosized font-sizes oscillate due to layouts caused by hovering or incremental page loading (and on other sites font sizes do eventually stabilize, but it takes many layouts before they reach a steady size). To prevent all these cases, we no longer allow the autosizing multiplier to change after it has been set (to a value other than 1). This won't always give exactly the same results, but testing on 2000 top sites shows that this makes little difference in practice, and it prevents these very jarring cases. As a happy side-effect, this speeds up layouts as font sizes change less. >> Source/WebCore/page/Settings.cpp:369 >> + // TODO: I wonder if this needs to traverse frames like in WebViewImpl::resize, or whether there is only one document per Settings instance? > > s/TODO/FIXME/ according to WebKit style. done >> Source/WebCore/rendering/TextAutosizer.cpp:99 >> + setMultiplier(renderer, 1); > > Please add a check for renderer->style()->textAutosizingMultiplier() != 1, as setting the multiplier unnecessarily is quite expensive. done >> Source/WebCore/rendering/TextAutosizer.cpp:199 >> + if (localMultiplier != descendant->style()->textAutosizingMultiplier() > > Nit: might be simpler/cheaper to write: localMultiplier != 1 && descendant->style()->textAutosizingMultiplier() == 1 done >> LayoutTests/ChangeLog:8 >> + Changed old tests to layout without the scrollbar initially, because the > > A little unclear. Perhaps change this sentence to "Added overflow-y:hidden to some existing tests, since previously those tests would start off with incorrect multipliers (because mainFrame->view()->layoutSize() is initially 785 instead of 800 as ScrollView wrongly guesses a scrollbar will be needed), and then the multipliers would get corrected on a subsequent layout. Now that we don't allow the multiplier to change after being set, it needs to be right first time." done >> LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:42 >> +if(element.offsetHeight){ > > Nit: space before ( and { please (ditto below). done >> LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:43 >> + // force layout (to computation of offsetHeight triggers reflow) > > Nit: s/to // done >> LayoutTests/fast/text-autosizing/oscillation-javascript-fontsize-change.html:45 >> +element.className='largersize'; > > Nit: spaces around = please (ditto below). done

Created attachment 188611 [details] Patch

Latest patch looks good to me. Kenneth/Julien, WDYT?

Comment on attachment 188611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188611&action=review > Source/WebCore/ChangeLog:12 > + eventually stabilize, but it takes many layouts before they reach a steady > + size). To prevent all these cases, we no longer allow the autosizing > + multiplier to change after it has been set (to a value other than 1). Sounds like an OK idea. > Source/WebCore/ChangeLog:17 > + This won't always give exactly the same results, but testing on 2000 top > + sites shows that this makes little difference in practice, and it prevents > + these very jarring cases. As a happy side-effect, this speeds up layouts > + as font sizes change less. What happens if you load the page in portrait, then rotate. in comparison with loading it in landscape and rotating to portrait? You get the same result?

Comment on attachment 188611 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188611&action=review >> Source/WebCore/ChangeLog:17 >> + as font sizes change less. > > What happens if you load the page in portrait, then rotate. in comparison with loading it in landscape and rotating to portrait? You get the same result? Very perceptive - this was in fact a bug with an initial implementation of this :) That's why the patch adds some code to chromium's WebViewImpl::resize which calls recalculateMultipliers if the page changes width, hence reseting the multiplier lock. However it would probably be better if this was done automatically from WebCore. Can you think of a good place to hook this in? Perhaps in FrameView::contentsResized?

> Very perceptive - this was in fact a bug with an initial implementation of this :) > That's why the patch adds some code to chromium's WebViewImpl::resize which calls recalculateMultipliers if the page changes width, hence reseting the multiplier lock. > However it would probably be better if this was done automatically from WebCore. Can you think of a good place to hook this in? Perhaps in FrameView::contentsResized? Yeah, I suppose FrameView would be the right place. I guess that way is should also be easier to test for all ports.

Created attachment 188903 [details] Patch

Comment on attachment 188903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188903&action=review > Source/WebCore/page/FrameView.cpp:450 > +#if ENABLE(TEXT_AUTOSIZING) > + // Autosized font sizes depend on the width of the viewing area. > + if (newRect.width() != oldRect.width()) { > + Page* page = m_frame ? m_frame->page() : 0; I think we need to do this elsewhere. Some ports don't use the FrameView as the viewport. There much be another place where you can get the content size changes.

Comment on attachment 188903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=188903&action=review >> Source/WebCore/page/FrameView.cpp:450 >> + Page* page = m_frame ? m_frame->page() : 0; > > I think we need to do this elsewhere. Some ports don't use the FrameView as the viewport. There much be another place where you can get the content size changes. This isn't looking for content size changes, it's looking for changes in the browser window width (such as when a device is rotated from portrait to landscape). I think we agreed on IRC that specifically the unscaledVisibleContentSize of the main frame's FrameView is the width we want to watch, and indeed that matches how TextAutosizer::processSubtree calculates the windowSize. Now ScrollView::unscaledVisibleContentSize is somewhat complicated, but it seems it primarily derives its value from Widget::width() and Widget::height(), which should always get changed via FrameView::setFrameRect. So perhaps this is a good place to detect width changes after all?

I am adding Dongsung as he was looking into fixed layout and setViewportSize.

(In reply to comment #15) > (From update of attachment 188903 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=188903&action=review > > >> Source/WebCore/page/FrameView.cpp:450 > >> + Page* page = m_frame ? m_frame->page() : 0; > > > > I think we need to do this elsewhere. Some ports don't use the FrameView as the viewport. There much be another place where you can get the content size changes. > > This isn't looking for content size changes, it's looking for changes in the browser window width (such as when a device is rotated from portrait to landscape). I think we agreed on IRC that specifically the unscaledVisibleContentSize of the main frame's FrameView is the width we want to watch, and indeed that matches how TextAutosizer::processSubtree calculates the windowSize. > > Now ScrollView::unscaledVisibleContentSize is somewhat complicated, but it seems it primarily derives its value from Widget::width() and Widget::height(), which should always get changed via FrameView::setFrameRect. So perhaps this is a good place to detect width changes after all? Hi! great work! and I have some questions. Do we need to call TextAutosizer::recalculateMultipliers() when viewport is changed? If so, I think we need to check when page scale is changed as well as FrameView::setFrameRect. I think the unscaledVisibleContentSize of the main frame's FrameView is nothing. the visibleContentRect of the main frame view said what is viewport rect in contents unit. the unscaledVisibleContentSize is just helper function for compositor. never mind unscaledVisibleContentSize. There is two factors that affect the visibleContentRect: frameRect and pageScale. Chromium compositor will stretch the visibleContentRect in contents unit to the device viewport in window (may multiply pageScale x deviceScale). EFL and Qt will follow Chromium's way. I'm not familiar to font autosize, so I'm not sure whether we should call TextAutosizer::recalculateMultipliers() when page scale is changed. WDYT? :)

from the http://trac.webkit.org/wiki/ScalesAndZooms: "Page scale is used to implement zooming gestures. [...] Page scale affects the virtual viewport, not the CSS viewport, so layout is, generally speaking, not impacted by a changing page scale." so if the layout is not impacted then we should not recompute autosizing when page scale changes.

(In reply to comment #18) > from the http://trac.webkit.org/wiki/ScalesAndZooms: > "Page scale is used to implement zooming gestures. [...] > Page scale affects the virtual viewport, not the CSS viewport, so layout is, generally speaking, not impacted by a changing page scale." > > so if the layout is not impacted then we should not recompute autosizing when page scale changes. ah, thank you for explanation. Text Autosizing is related to layout rather than virtual viewport. I agree with you that FrameView::setFrameRect is relevant position. Frankly, I don't fully understand what is Text Autosizing and css viewport yet. :)

> Do we need to call TextAutosizer::recalculateMultipliers() when viewport is changed? It needs to be recalcuted whenever the visible native viewport changes width, disregarding scaling. > ah, thank you for explanation. Text Autosizing is related to layout rather than virtual viewport. I agree with you that FrameView::setFrameRect is relevant position. Frankly, I don't fully understand what is Text Autosizing and css viewport yet. :) Not really, it is related to what we call the initial layout viewport (according to the css device adaptation spec). which means the visible viewport at css scale 1.0. If this changes, like as a result of an orientation change, then it needs to be recalculated.

(In reply to comment #20) Or to put it another way, Text Autosizing depends on the physical width of the device, measured in density-independent pixels (DIP). For example a portrait iPhone is 320 DIP wide (whether it's high dpi or low dpi), and a landscape iPhone 4 is 480 DIP wide. This is irrespective of the page's meta viewport tag or zoom level. When the device gets rotated from portrait to landscape, and the window width switches from e.g. 320 DIP to 480 DIP, we need to call recalculateMultipliers. TextAutosizer::processSubtree currently accesses this DIP window width using page->mainFrame()->view()->unscaledVisibleContentSize(true). That definitely works on chromium (both android and linux); if it turns out that doesn't work on other ports which do tiling outside of WebKit then we should change that, but it would probably be cleaner to do so as a separate bug/patch?

Comment on attachment 188903 [details] Patch I am ok with this, but it would be great if we could actually test by modifying the viewport. That can be done from testing already I believe, but we need to make sure it uses fixed layout for EFL.

Comment on attachment 188903 [details] Patch Clearing flags on attachment: 188903 Committed r143356: <http://trac.webkit.org/changeset/143356>

All reviewed patches have been landed. Closing bug.

(In reply to comment #21) > (In reply to comment #20) > TextAutosizer::processSubtree currently accesses this DIP window width using page->mainFrame()->view()->unscaledVisibleContentSize(true). That definitely works on chromium (both android and linux); if it turns out that doesn't work on other ports which do tiling outside of WebKit then we should change that, but it would probably be cleaner to do so as a separate bug/patch? I understand now page->mainFrame()->view()->unscaledVisibleContentSize(true) returns css viewport, that http://trac.webkit.org/wiki/ScalesAndZooms and http://dev.w3.org/csswg/css-device-adapt/ said. kenneth, when we use fixedLayoutSize, is css viewport determined by fixedLayoutSize instead of frameRect? If so, I think we need to call TextAutosizer::recalculateMultipliers() when fixedLayoutSize is changed..

(In reply to comment #25) > (In reply to comment #21) > > (In reply to comment #20) > > TextAutosizer::processSubtree currently accesses this DIP window width using page->mainFrame()->view()->unscaledVisibleContentSize(true). That definitely works on chromium (both android and linux); if it turns out that doesn't work on other ports which do tiling outside of WebKit then we should change that, but it would probably be cleaner to do so as a separate bug/patch? > > I understand now page->mainFrame()->view()->unscaledVisibleContentSize(true) returns css viewport, that http://trac.webkit.org/wiki/ScalesAndZooms and http://dev.w3.org/csswg/css-device-adapt/ said. CSS viewport at 1.0 scale, yes. > kenneth, when we use fixedLayoutSize, is css viewport determined by fixedLayoutSize instead of frameRect? If so, I think we need to call TextAutosizer::recalculateMultipliers() when fixedLayoutSize is changed.. fixed layout size is not equal to the viewport at 1.0 scale (only when width=device-width). Often the content is laid out so that it does not fit into the viewport, given fallback values (say 980 for desktop pages) or given values from the viewport meta tag.

(In reply to comment #26) > fixed layout size is not equal to the viewport at 1.0 scale (only when width=device-width). Often the content is laid out so that it does not fit into the viewport, given fallback values (say 980 for desktop pages) or given values from the viewport meta tag. aha, Thanks for explanation. fixed layout size and css viewport makes me confused :) In detail, I'm trying to understand which code of WebCore use fixed layout size, css viewport (unscaledVisibleContentSize) or just Widget::frameRect.