Do not let text-size-adjust override accessibility font settings

Do not let text-size-adjust override accessibility font settings

The text autosizer is used for both making desktop pages more legible
on mobile, and for applying the accessibility font scale factor. When
text-size-adjust support was added, pages were able to override the
accessibility setting in addition to the autosizing multiplier.

This patch ensures accessibility font scale settings are respected even
with text-size-adjust. A followup bug has been filed (crbug.com/645717)
for moving the accessibility font scale factor out of the autosizer.

This patch also re-enables support for text-size-adjust which was
disabled temporarily due to breaking accessibility settings.

BUG=645269
Committed: https://crrev.com/eccac1fd6db85731fb95b11915fc2197a6ceb0c3
Cr-Commit-Position: refs/heads/master@{#418173}

[pdr., 2016-09-10 03:17:13]

Description was changed from

==========
Do not let text-size-adjust override accessibility font settings

The text autosizer is used for both making desktop pages more legible
on mobile, and for applying the accessibility font scale factor. When
text-size-adjust support was added, pages were able to override the
accessibility setting in addition to the autosizing multiplier.

This patch ensures accessibility font scale settings are respected even
with text-size-adjust. A followup bug has been filed (crbug.com/645717)
for moving the accessibility font scale factor out of the autosizer.

BUG=645269
==========

to

==========
Do not let text-size-adjust override accessibility font settings

The text autosizer is used for both making desktop pages more legible
on mobile, and for applying the accessibility font scale factor. When
text-size-adjust support was added, pages were able to override the
accessibility setting in addition to the autosizing multiplier.

This patch ensures accessibility font scale settings are respected even
with text-size-adjust. A followup bug has been filed (crbug.com/645717)
for moving the accessibility font scale factor out of the autosizer.

This patch also re-enables support for text-size-adjust which was
disabled temporarily due to breaking accessibility settings.

BUG=645269
==========

[pdr., 2016-09-10 03:17:13]

pdr@chromium.org changed reviewers:
+ skobes@chromium.org

[pdr., 2016-09-10 03:26:17]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-10 03:26:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-10 05:03:33]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-10 05:03:34]

Dry run: This issue passed the CQ dry run.

[skobes, 2016-09-12 21:45:44]

https://codereview.chromium.org/2329733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right):

https://codereview.chromium.org/2329733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/TextAutosizer.cpp:566:
m_pageInfo.m_pageNeedsAutosizing = !!m_pageInfo.m_frameWidth
Out of scope for this change, but the logic around m_pageNeedsAutosizing
probably needs rethinking for text-size-adjust, in two ways:

- I think we will fail to boost when we should, if the page sets
width=device-width and text-size-adjust > 100% (because m_pageNeedsAutosizing
will be false)?

- When text-size-adjust has a document-wide value other than "auto",
TextAutosizer is doing a lot of unnecessary work to compute the auto multipliers
only to ignore them in applyMultiplier.  It would be nice to short circuit this
somehow.

https://codereview.chromium.org/2329733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/TextAutosizer.cpp:998: multiplier =
currentStyle.getTextSizeAdjust().multiplier() *
m_pageInfo.m_accessibilityFontScaleFactor;
Just to confirm, the reason you're adding a new field instead of using
m_baseMultiplier is that you don't want to include the device scale adjustment?

In that case it may be clearer to store m_deviceScaleAdjustment in PageInfo
instead of m_baseMultiplier.

[pdr., 2016-09-13 03:31:19]

https://codereview.chromium.org/2329733002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right):

https://codereview.chromium.org/2329733002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/TextAutosizer.cpp:998: multiplier =
currentStyle.getTextSizeAdjust().multiplier() *
m_pageInfo.m_accessibilityFontScaleFactor;
On 2016/09/12 at 21:45:44, skobes wrote:
> Just to confirm, the reason you're adding a new field instead of using
m_baseMultiplier is that you don't want to include the device scale adjustment?
> 
> In that case it may be clearer to store m_deviceScaleAdjustment in PageInfo
instead of m_baseMultiplier.

Good idea, I've split out m_baseMultiplier into m_accessibilityFontScaleFactor
and m_deviceScaleAdjustment

[pdr., 2016-09-13 03:36:23]

On 2016/09/12 at 21:45:44, skobes wrote:
>
https://codereview.chromium.org/2329733002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/layout/TextAutosizer.cpp (right):
> 
>
https://codereview.chromium.org/2329733002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/layout/TextAutosizer.cpp:566:
m_pageInfo.m_pageNeedsAutosizing = !!m_pageInfo.m_frameWidth
> Out of scope for this change, but the logic around m_pageNeedsAutosizing
probably needs rethinking for text-size-adjust, in two ways:
> 
> - I think we will fail to boost when we should, if the page sets
width=device-width and text-size-adjust > 100% (because m_pageNeedsAutosizing
will be false)?

Good catch. In the latest patchset I added a (disabled) test, a todo, and filed
https://crbug.com/646237.

Before shipping I text-size-adjust I checked the stats on HTTPArchive and
essentially all existing usage (I forget the exact number) was for disabling
autosizing. For this reason, I'm not too worried about this affecting users, but
it is a bug.

> 
> - When text-size-adjust has a document-wide value other than "auto",
TextAutosizer is doing a lot of unnecessary work to compute the auto multipliers
only to ignore them in applyMultiplier.  It would be nice to short circuit this
somehow.

+1

[pdr., 2016-09-13 03:36:28]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[pdr., 2016-09-13 03:36:43]

PTAL

[commit-bot: I haz the power, 2016-09-13 03:36:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[skobes, 2016-09-13 04:11:53]

lgtm

[commit-bot: I haz the power, 2016-09-13 04:59:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 04:59:18]

Dry run: This issue passed the CQ dry run.

[pdr., 2016-09-13 05:10:33]

On 2016/09/13 at 04:11:53, skobes wrote:
> lgtm

Thanks!

[pdr., 2016-09-13 05:10:34]

The CQ bit was checked by pdr@chromium.org

[commit-bot: I haz the power, 2016-09-13 05:10:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 05:14:10]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-13 05:16:15]

Description was changed from

==========
Do not let text-size-adjust override accessibility font settings

The text autosizer is used for both making desktop pages more legible
on mobile, and for applying the accessibility font scale factor. When
text-size-adjust support was added, pages were able to override the
accessibility setting in addition to the autosizing multiplier.

This patch ensures accessibility font scale settings are respected even
with text-size-adjust. A followup bug has been filed (crbug.com/645717)
for moving the accessibility font scale factor out of the autosizer.

This patch also re-enables support for text-size-adjust which was
disabled temporarily due to breaking accessibility settings.

BUG=645269
==========

to

==========
Do not let text-size-adjust override accessibility font settings

The text autosizer is used for both making desktop pages more legible
on mobile, and for applying the accessibility font scale factor. When
text-size-adjust support was added, pages were able to override the
accessibility setting in addition to the autosizing multiplier.

This patch ensures accessibility font scale settings are respected even
with text-size-adjust. A followup bug has been filed (crbug.com/645717)
for moving the accessibility font scale factor out of the autosizer.

This patch also re-enables support for text-size-adjust which was
disabled temporarily due to breaking accessibility settings.

BUG=645269

Committed: https://crrev.com/eccac1fd6db85731fb95b11915fc2197a6ceb0c3
Cr-Commit-Position: refs/heads/master@{#418173}
==========

[commit-bot: I haz the power, 2016-09-13 05:16:16]

Patchset 2 (id:??) landed as
https://crrev.com/eccac1fd6db85731fb95b11915fc2197a6ceb0c3
Cr-Commit-Position: refs/heads/master@{#418173}

[aberent, 2016-09-13 17:05:07]

A revert of this CL (patchset #2 id:20001) has been created in
https://codereview.chromium.org/2335193003/ by aberent@chromium.org.

The reason for reverting is: Broke
ContextualSearchManagerTest#testChainedSearchCreatesNewContent and
ContextualSearchManagerTest#testChainedTapsRemovedFromHistory

BUG=646342.