[Webkit-unassigned] [Bug 108205] Text Autosizing: prevent oscillation of font sizes during autosizing

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=108205





--- Comment #7 from timvolodine at chromium.org  2013-02-15 11:46:09 PST ---
(From update of attachment 188593)
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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.