1340206 - Compact Themes makes text on location bar to look blurry/bouncy when hovering with RTL builds

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P2)

Description

[Ciprian Georgiu, Desktop QA]

Attachment: compact themes - location bar issue.gif

[Affected versions]:
- latest Nightly 54.0a1
- latest Aurora 53.0a2

[Affected platforms]:
- Windows 10, 7, 8.1 x64
- Mac OS X 10.11.6
- Ubuntu 16.04 x64 LTS

[Steps to reproduce]:
1. Download and start Firefox on an RTL build from here: https://archive.mozilla.org/pub/firefox/nightly/2017/02/2017-02-16-03-02-10-mozilla-central-l10n/ 
2. Enable Compact Dark or Compact Light theme from about:addons 
3. Hover with your mouse over the location bar 

[Expected result]:
- Text in the location bar is properly displayed and no blurry/bouncy effect can be seen 

[Actual result]:
- Text in the location bar appears blurry/bouncy when hovering over it

[Regression range]:
- This is not a regression as I can reproduce this on Nightly from 15-01-2017 where this feature first landed.

[Additional notes]:
- I've managed to reproduce this with RTL builds like: ".ar", ".fa" or ".he". With other lightweight themes or Default theme it's not reproducible.
- also you can hit this with Force RTL addon from amo, but it seems that, in this case only latest Aurora 53.0a2 is affected

Comment 1

[:Gijs (he/him)]

Does this reproduce with older nightlies when using the devedition theme (before it became a compact theme) ?

Comment 2

[Ciprian Georgiu, Desktop QA]

Yes it does. I've reproduce this with an old Nightly 45.0a1 (2015-11-04) having the Dev Edition theme applied.

Comment 3

[:Gijs (he/him)]

I've no idea what's causing this. Jeff, how do you feel about priority here? Doesn't look like it's very nice behaviour for people on RTL locales...

Comment 4

[Jeff Griffiths (:canuckistani) (:⚡︎)]

I'd like to get this fixed, and I agree it's probably the most important bug we have. I don't think it should block though.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking since it's something we should probably fix before we release this feature.

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I am able to reproduce this issue here on macOS.

After a fun session of bisecting browser styles, it seems related to the following rules:

window:not([chromehidden~="toolbar"]) #urlbar-wrapper:-moz-locale-dir(rtl) { // browser.css
    -moz-box-direction: reverse;
}

window:not([chromehidden~="toolbar"]) #urlbar-wrapper:-moz-locale-dir(rtl), 
window:not([chromehidden~="toolbar"]) #urlbar-wrapper > #urlbar:-moz-locale-dir(rtl) { // browser.css
    transform: scaleX(-1);
}

window:not([chromehidden~="toolbar"]) #urlbar-wrapper { // compacttheme.css
    overflow: -moz-hidden-unscrollable;
    clip-path: none;
    margin-inline-start: 0;
}

window:not([chromehidden~="toolbar"]) #urlbar-wrapper { // browser.css
    clip-path: url("chrome://browser/content/browser.xul#urlbar-back-button-clip-path");
    margin-inline-start: calc(-1 * var(--backbutton-urlbar-overlap));
}

If I use the inspector to add `transform: scaleX(-1);` to non-RTL, then the issue appears in my en-US build.

It seems that adding `will-change: transform;` solves the issue here.

Although `transform: scaleX(-1);` is set by regular browser styles (not Compact Theme), the issue does not seem to appear in the Default Theme.  Compact Theme's `clip-path: none;` also needs to be present to trigger the issue.

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1340206 - Force compositing layer to avoid blurry RTL URL bar.

With Compact Themes and RTL locales, the URL bar becomes blurry on hover.  This
seems to be triggered by the combination:

```
transform: scaleX(-1);
clip-path: none;
```

By forcing a compositing layer, the blurry issue goes away.

Review commit: https://reviewboard.mozilla.org/r/115474/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115474/

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Gijs, is there a way to produce an RTL / l10n build on try?  Or who might know how to do it?

Comment 9

[:Gijs (he/him)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8)
> Gijs, is there a way to produce an RTL / l10n build on try?  Or who might
> know how to do it?

I'm not sure about try, actually. Maybe :flod knows.

Comment 10

[Francesco Lodolo [:flod]]

Sadly I have no familiarity with the build system and try.

There are instructions here, but I have no way to tell if they're up to date
https://wiki.mozilla.org/ReleaseEngineering/TryServer#Desktop_l10n_jobs

If this doesn't work, :callek might have some insights since he's doing work on l10n build.

Comment 11

[:Gijs (he/him)]

Comment on attachment 8841143 [details]
Bug 1340206 - Force compositing layer to avoid blurry RTL URL bar.

https://reviewboard.mozilla.org/r/115474/#review117058

I trust your ability to test this and therefore r=me for this as a "it makes it not be buggy" fix. But I don't *understand* why the CSS in question triggers this bug, and it feels like that would be a layout bug that we should address independently. :dholbert has in the past been interested in such quirks, maybe he can help figure out what's going on (and potentially get it fixed in a separate bug).

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs from comment #11)
> Comment on attachment 8841143 [details]
> Bug 1340206 - Force compositing layer to avoid blurry RTL URL bar.
> 
> https://reviewboard.mozilla.org/r/115474/#review117058
> 
> I trust your ability to test this and therefore r=me for this as a "it makes
> it not be buggy" fix. But I don't *understand* why the CSS in question
> triggers this bug, and it feels like that would be a layout bug that we
> should address independently. :dholbert has in the past been interested in
> such quirks, maybe he can help figure out what's going on (and potentially
> get it fixed in a separate bug).

--> ni dholbert

Comment 13

[Daniel Holbert [:dholbert]]

What are the CSS changes that get triggered when we hover?  Hard to understand what's going on without knowing that.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Daniel Holbert [:dholbert] from comment #13)
> What are the CSS changes that get triggered when we hover?  Hard to
> understand what's going on without knowing that.

Well, that's where I get more confused...  Forcing hover state with the inspector does not seem to make any additional styles appear for this element, but it's clear that hovering triggers the blurry behavior.  Maybe there is a hover rule somewhere else in hierarchy...?

The "fuzzy text" behavior is actually triggered before I get to the #urlbar-wrapper element itself.  It seems to happen once I move within one of its parents #navigator-toolbox (this is the container for everything above the content, such as tabs, URL bar, etc).

The #navigator-toolbox styles appear to be:

#navigator-toolbox, 
#browser-bottombox, 
#content-deck {
    transition-property: margin-left, margin-right;
    transition-duration: 200ms;
    transition-timing-function: linear;
}
#navigator-toolbox, 
#mainPopupSet {
    min-width: 1px;
}
toolbox {
    -moz-appearance: toolbox;
}

However, the text fuzzing is still triggered even if I disable all of the above styles on #navgiator-toolbox, so it suggests they are not related.

Comment 15

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

On macOS at least, it looks related to font anti-aliasing behavior (or it seems that way when using the xScope tool to magnify the screen).

In the default state, the font in the location bar seems to use various colored pixels for anti-aliasing.  When I hover within #navigator-toolbox, the extra pixels for anti-aliasing change to grayscale instead for a few seconds, but then flip back to color.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I was able to produce try build for he locale on Linux (:Callek told me other platforms would timeout).

https://queue.taskcluster.net/v1/task/bw2SWIqFSVeeBoIA1gYt2w/runs/0/artifacts/public/build/he/target.tar.bz2

Ciprian, can you confirm that the issue seems fixed, at least on Linux, with the try build above?  It seems fixed to me, but it's a subtle thing, so I'd like to be confident before landing.

Comment 17

[Daniel Holbert [:dholbert]]

(Punting needinfo to mattwoodrow, since he knows the layerization/AA details better than I do.)

Comment 18

[Matt Woodrow (:mattwoodrow)]

This seems to be a case of translating something by a subpixel amount, causing us to lose subpixel-AA.

Seems like there's at least a few layout bugs here:

* Static translation-only transforms by non-integer amounts are causing us to lose subpixel-AA. We should only be disabling subpixel-AA for active transforms (will-change, or animated) [1].

* Changing from an integer translation to a non-integer translation on an already active transform (use will-change) doesn't result in us trigger an invalidation to remove the subpixel-AA. We use the existing retained contents, but the subpixel-AA colors will be off since we're copying them by a subpixel amount. When we trigger mDisableSubpixelAntialiasingInDescendants=true, then we call DisableComponentAlpha() on display items. I think nsDisplayText::ComputeInvalidationRegion needs to check if this state has changed, and invalidate itself if so.

Note that this will likely break the workaround proposed in this bug.

* If a transform has an async animation, then we should always disable subpixel-AA, since it could change to be a non-integer translation at any point and we won't get a chance to invalidate it.

Thinker, is this something you'd be interested in picking up?





[1] http://searchfox.org/mozilla-central/source/layout/painting/FrameLayerBuilder.cpp#5502

Comment 19

[Daniel Holbert [:dholbert]]

(In reply to Matt Woodrow (:mattwoodrow) from comment #18)
> * Changing from an integer translation to a non-integer translation on an
> already active transform (use will-change) doesn't result in us trigger an
> invalidation to remove the subpixel-AA.

I filed bug 1343057 on this, BTW.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Matt, we'd like to uplift a fix for the visual issue in this bug to 53.  Do you think the AA issues you've outlined in comment 18 can be fixed and uplifted to 53?  Or should I land the workaround in 53+ for now, and then we can revert the workaround in whichever release fixes the underlying AA issues?

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:ntim pointed out on IRC that we should be able to remove the `transform` rule for Compact Theme, since it was only used to position the `clip-path` for RTL.

I'll post a new build with this approach once Try opens again.

Comment 22

[Ciprian Georgiu, Desktop QA]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
...
> Ciprian, can you confirm that the issue seems fixed, at least on Linux, with
> the try build above?  It seems fixed to me, but it's a subtle thing, so I'd
> like to be confident before landing.

Now, the visual issue seems very subtle on Linux with the try build, but I can no longer see the blurry/bouncy effect. I think, I am seeing the issues mentioned in comment 18, and filed in bug 1343057. You can see here what it looks like on my machine: https://drive.google.com/file/d/0B6qEQD4BO4AlVjQ1cDJhV01oaVU/view

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Disable transform to avoid blurry RTL URL bar

(Pushing to MozReview gives a 500 error, so let's try patches.)

I've switched to :ntim's suggested approach here.  We don't actually need the transform in Compact Themes.

This avoids the downside with the previous version (subpixel AA works as expected with this approach).

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Clearing ni? for Matt since the new approach seems to avoid the platform issues.

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Ciprian, thanks for the previous test!

I have another version based on a new approach, which should keep subpixel AA working correctly.  Can you try this one as well?

https://queue.taskcluster.net/v1/task/PJ-JLhf_SrKPXI1bO67v4w/runs/0/artifacts/public/build/he/target.tar.bz2

Comment 26

[:Gijs (he/him)]

Comment on attachment 8842624 [details] [diff] [review]
Disable transform to avoid blurry RTL URL bar

Review of attachment 8842624 [details] [diff] [review]:
-----------------------------------------------------------------

r=me

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ddfa8072f3
Disable transform to avoid blurry RTL URL bar. r=Gijs

Comment 28

[Thinker Li [:sinker]]

clear ni.. I will leave messages on bug 1343057.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/42ddfa8072f3

Comment 30

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b946ece0c9d2bf34a925d13d08be2724f275dfd5

Comment 31

[Ciprian Georgiu, Desktop QA]

Sorry for my late reply.

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #25)
> Ciprian, thanks for the previous test!
> 
> I have another version based on a new approach, which should keep subpixel
> AA working correctly.  Can you try this one as well?
> 
> https://queue.taskcluster.net/v1/task/PJ-JLhf_SrKPXI1bO67v4w/runs/0/
> artifacts/public/build/he/target.tar.bz2

It looks good now with the above build.

Also, I can no longer reproduce this issue on RTL builds (.ar, .fa, or .he) with latest Nightly 54.0a1 (2017-03-06) and latest Aurora 53.0a2 (2017-03-06). Tests were done under Windows 10 x64, Ubuntu 16.04 x64 LTS and Mac OS X 10.11.6.

Thanks!