Closed Bug 1340206 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Theme, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 54
Tracking Status
firefox53 + verified
firefox54 + verified

People

(Reporter: ciprian_georgiu, Assigned: jryans)

References

Details

Attachments

(2 files, 1 obsolete file)

[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
Has STR: --- → yes
Does this reproduce with older nightlies when using the devedition theme (before it became a compact theme) ?
Flags: needinfo?(ciprian.georgiu)
Yes it does. I've reproduce this with an old Nightly 45.0a1 (2015-11-04) having the Dev Edition theme applied.
Flags: needinfo?(ciprian.georgiu)
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...
Blocks: 1331679
Flags: needinfo?(jgriffiths)
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.
Flags: needinfo?(jgriffiths)
Priority: -- → P2
Tracking since it's something we should probably fix before we release this feature.
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.
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Gijs, is there a way to produce an RTL / l10n build on try?  Or who might know how to do it?
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(francesco.lodolo)
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.
Flags: needinfo?(francesco.lodolo)
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).
Attachment #8841143 - Flags: review?(gijskruitbosch+bugs) → review+
(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
Flags: needinfo?(dholbert)
What are the CSS changes that get triggered when we hover?  Hard to understand what's going on without knowing that.
Flags: needinfo?(dholbert) → needinfo?(jryans)
(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.
Flags: needinfo?(jryans) → needinfo?(dholbert)
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.
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.
Flags: needinfo?(ciprian.georgiu)
(Punting needinfo to mattwoodrow, since he knows the layerization/AA details better than I do.)
Flags: needinfo?(dholbert) → needinfo?(matt.woodrow)
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
Flags: needinfo?(matt.woodrow) → needinfo?(tlee)
(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.
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?
Flags: needinfo?(matt.woodrow)
: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.
(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
Flags: needinfo?(ciprian.georgiu)
(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).
Attachment #8841143 - Attachment is obsolete: true
Attachment #8842624 - Flags: review?(gijskruitbosch+bugs)
Clearing ni? for Matt since the new approach seems to avoid the platform issues.
Flags: needinfo?(matt.woodrow)
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
Flags: needinfo?(ciprian.georgiu)
Comment on attachment 8842624 [details] [diff] [review]
Disable transform to avoid blurry RTL URL bar

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

r=me
Attachment #8842624 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/42ddfa8072f3
Disable transform to avoid blurry RTL URL bar. r=Gijs
clear ni.. I will leave messages on bug 1343057.
Flags: needinfo?(tlee)
https://hg.mozilla.org/mozilla-central/rev/42ddfa8072f3
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
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!
Status: RESOLVED → VERIFIED
Flags: needinfo?(ciprian.georgiu)
You need to log in before you can comment on or make changes to this bug.