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

VERIFIED FIXED in Firefox 53

Status

()

P2
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: ciprian_georgiu, Assigned: jryans)

Tracking

(Blocks: 1 bug)

Trunk
Firefox 54
Points:
---

Firefox Tracking Flags

(firefox53+ verified, firefox54+ verified)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 8838116 [details]
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
Has STR: --- → yes

Comment 1

2 years ago
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)

Comment 3

2 years ago
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.
tracking-firefox53: --- → +
tracking-firefox54: --- → +
(Assignee)

Comment 6

2 years ago
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
Comment hidden (mozreview-request)
(Assignee)

Comment 8

2 years ago
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)

Comment 9

2 years ago
(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 11

2 years ago
mozreview-review
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+

Comment 12

2 years ago
(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)
(Assignee)

Comment 14

2 years ago
(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)
(Assignee)

Comment 15

2 years ago
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.
(Assignee)

Comment 16

2 years ago
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.
(Assignee)

Comment 20

2 years ago
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)
(Assignee)

Comment 21

2 years ago
: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)
(Assignee)

Comment 23

2 years ago
Created attachment 8842624 [details] [diff] [review]
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).
Attachment #8841143 - Attachment is obsolete: true
Attachment #8842624 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 24

2 years ago
Clearing ni? for Matt since the new approach seems to avoid the platform issues.
Flags: needinfo?(matt.woodrow)
(Assignee)

Comment 25

2 years ago
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 26

2 years ago
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+

Comment 27

2 years ago
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)

Comment 29

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42ddfa8072f3
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox54: affected → fixed
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
status-firefox53: fixed → verified
status-firefox54: fixed → verified
Flags: needinfo?(ciprian.georgiu)
You need to log in before you can comment on or make changes to this bug.