1355991 - Focus outline performs outline-width transition from 3px to 1px once element is focused (if transition is set)

Status: NEW

(Core :: CSS Parsing and Computation, defect, P3)

Description

[684sigma]

I have a problem with Beta 53. It also happens in Nightly 55. Doesn't happen in Beta 52, ESR 45.
When I focus some elements, focus outline performs quick distracting transition from 3px to 1px. It happens in Customize (menu->customize) and even on Bugzilla. Here's how to reproduce the bug:

1. Open url - data:text/html,<a href="https://bugzilla.mozilla.org/" style="transition:all 100ms ease">The long link</a>
2. Press Tab to focus the link

Result: every time focus outline performs distracting transition from 3px to 1px once the link is focused
Expected: no transition

Comment 1

[684sigma]

Mozregression-gui generated this regression range:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=13bdf07a0811ae84491d7c23fc6a372dbdaccc6e&tochange=95c8f7a8569e36d7902c954d74e26a3851e4f29c
->
1320239 – Use nscoord for some nsStyleCoord in style structs that only carry nscoord
https://bugzilla.mozilla.org/show_bug.cgi?id=1320239

Comment 2

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Too late for a fix in 53 and likely too late for 54.

Comment 3

[Astley Chen (inactive)]

Jeremy, let's figure out the real root cause of this bug.

Comment 4

[Jeremy Chen ([e-mail])]

Ok, at first, I don't think we should have any transition/animation effect if we do keep outline-width as its default value. I'm sure my implementation did maintain the correctness of the outline-width's initial value. So, I dug a little and found that the style setting for focused hyperlink is "outline: 1px dotted;" [1]. Hmmm... so maybe the animation/transition is expected? Then, I wonder why the original design didn't have this kind of animation/transition. It turns out that we stored 'medium' (a nsStyleCoord with eStyleUnit_Enumerated) as outline-width's initial value, and the animation type was eStyleAnimType_Coord before. So, maybe there was a short flip-like transition/animation (from 'medium' to '1px') before, it was just not that obvious.

However, in Bug 1320239, we use nscoord instead of nsStyleCoord to store computed value for outline-width. So, the animation type is also changed from eStyleAnimType_Coord to eStyleAnimType_nscoord [2]. The transition/animation is now from 3px to 1px, which is more obvious than before, and may not be expected.

After discussed with Cameron and Brian (cc both of them), we could use a special internal keyword (or a boolean) to avoid doing transition/animation for this kind of situation.


[1] https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/layout/style/res/ua.css#128-131
[2] https://searchfox.org/mozilla-central/rev/17ebac68112bd635b458e831670c1e506ebc17ad/layout/style/nsCSSPropList.h#3243

Comment 5

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 (away 7/3-7/11) from comment #4)
> After discussed with Cameron and Brian (cc both of them), we could use a
> special internal keyword (or a boolean) to avoid doing transition/animation
> for this kind of situation.

To be more clear, the special situation is that, if at least one of the To/From value of the outline-width property is unset/initial, we shall not do transition/animation at all.

Comment 6

[Jeremy Chen ([e-mail])]

Attachment: Bug 1355991 - avoid animation/transition from or to an initial/unset outline-width.

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

Comment 7

[Jeremy Chen ([e-mail])]

The current wip could fix the issue on my local machine.
Let's see how it goes on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=dbb8b7422bc4

Comment 8

[Jeremy Chen ([e-mail])]

Try looks good. Ask for review then.

I need some feedback from Brian as well:

1. I'd like to know if this is the right place to do the customization procedure for animation/transition for outline-width.

2. I'm not certain about the corresponding servo side change that I should make, might need a bit hint.
An idea is adding an extra boolean variable, just like what we do in gecko, and make some customization in servo side as well. But, that would complicate the glue code for sure. Also, I'm not familiar with animation/transition mechanism for stylo, so it would be helpful if you could point me where I should look into.

Comment 9

[Jeremy Chen ([e-mail])]

Comment on attachment 8882522 [details]
Bug 1355991 - avoid animation/transition from or to an initial/unset outline-width.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/153640/diff/1-2/

Comment 10

[Jeremy Chen ([e-mail])]

Per discussed with Cameron and Brian, since our current behavior is correct (and Chrome also does the same thing), another thing that we could try is to change 'medium' keyword for outline-width to be '1px' instead of '3px'. This should be doable since the implementation of 'medium' keyword is UA dependent.

Clear review requests for now.

Comment 11

[Jeremy Chen ([e-mail])]

Just tested on Chrome with

```
data:text/html,<a href="https://bugzilla.mozilla.org/" style="outline: medium solid; transition:all 1000ms ease">The long link</a>
```

Then, I run following command in inspector's console,
```
elt = document.getElementsByTagName('a')[0];
getComputedStyle(elt).getPropertyValue("outline-width");
```

and get '3px'.... seems agree with that in Firefox...

Comment 12

[Brian Birtles (:birtles)]

Hmm, I get '1.2px' on Windows in both canary and release.

Comment 13

[Jeremy Chen ([e-mail])]

(In reply to Brian Birtles (:birtles) from comment #12)
> Hmm, I get '1.2px' on Windows in both canary and release.

No idea what changed, but now I can get '0.6px' on Mac in both canary and release.

On Linux, I get '0.6px' in Chrome release.
Since Canary is not available on Linux, I manually pull and build the latest chromium on Linux, and get '0.6px' as well.

Another thing is, Chrome uses LayoutUnit(3) as the initial value of outline-width [1], which is exact the same as Gecko (3 app units) does. Not sure what magic trick does Blink/Webkit use to make the computed values platform-dependent....



[1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/CSSProperties.json5?type=cs&q=%22LayoutUnit(3)%22&l=1904

Comment 14

[Jeremy Chen ([e-mail])]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #13)
> (In reply to Brian Birtles (:birtles) from comment #12)
> > Hmm, I get '1.2px' on Windows in both canary and release.
> 
> No idea what changed, but now I can get '0.6px' on Mac in both canary and
> release.
> 
> On Linux, I get '0.6px' in Chrome release.
> Since Canary is not available on Linux, I manually pull and build the latest
> chromium on Linux, and get '0.6px' as well.
> 

Okay, looks like chrome would return different computed value if the global zoom factor is changed. This is the reason why I get different computed values for "outline-width: medium;". So, if the zoom factor is default (100%), the computed value of outline-width would be "3px" in both Firefox and Chrome (even Edge).

It's not easy to find a perfect solution here. So far, we have 2 possible approaches:

1. Change the keyword mappings just for outline-width.
To do so, we can map 'medium' to 1px, to avoid the transition from 3px to 1px. But, we will still see a transition from "outline-style: none;" to "outline-style: dotted;" (might be a flip transition effect I guess). Also, though we only change the keyword mappings for outline-width (keep the border-width as it is), there might be some consequences due to this change of mapping.

2. Patch UA stylesheet for link's default style [1].
To avoid the distracted transition, we can add "outline: transparent 1px dotted;" to link's default style. In this way, the transition would be nearly unobservable.

I personally a bit lean against the second approach, which IMO could avoid the weird dot transitioning totally.

Hi David, does the second approach make sense to you? I'd like to listen to your opinion before going further.



[1] https://searchfox.org/mozilla-central/rev/31311070d9860b24fe4a7a36976c14b328c16208/layout/style/res/ua.css#124-126

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

So it's not clear to me which configuration you're seeing problems in, and what it is you'd like to change.  We have this chunk of code:
https://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/layout/style/nsLayoutStylesheetCache.cpp#940-976

and I'm guessing (given the testcase in comment 11, which wouldn't do anything interesting in Firefox without the !important declaration introduced there -- although comment 11 didn't really refer to Firefox results so I'm not sure).


Maybe if we're hitting that chunk of code because of the value of focusRingWidth we shouldn't be adding !important, since then it would match:
https://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/layout/style/res/ua.css#128-131

It's not clear to me why we want such a big difference that's a function of wherever that width comes from.  (Or is there (or was there) another source of !important outline rules?)

Comment 16

[Jeremy Chen ([e-mail])]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #15)
> So it's not clear to me which configuration you're seeing problems in, and
> what it is you'd like to change.  We have this chunk of code:
> https://searchfox.org/mozilla-central/rev/
> cef8389c687203085dc6b52de2fbd0260d7495bf/layout/style/
> nsLayoutStylesheetCache.cpp#940-976
> 
> and I'm guessing (given the testcase in comment 11, which wouldn't do
> anything interesting in Firefox without the !important declaration
> introduced there -- although comment 11 didn't really refer to Firefox
> results so I'm not sure).
> 
> 
> Maybe if we're hitting that chunk of code because of the value of
> focusRingWidth we shouldn't be adding !important, since then it would match:
> https://searchfox.org/mozilla-central/rev/
> cef8389c687203085dc6b52de2fbd0260d7495bf/layout/style/res/ua.css#128-131
> 
> It's not clear to me why we want such a big difference that's a function of
> wherever that width comes from.  (Or is there (or was there) another source
> of !important outline rules?)

Hmmm, comment 11 is a testcase that we try to understand why the 'medium' keyword for outline-width is rendered different in Firefox and Chrome, which is just for discussing the possible solutions for this bug (we thought about mapping 'medium' to '1px' for outline-width property at that time). So, it might be a bit irrelevant to this bug. I should've put more context while requesting the needinfo.

Here's a testcase (an enhanced version of comment 0) that would reveal a bit more about the issue in this bug:

```
data:text/html,<a href="https://bugzilla.mozilla.org/" style="transition:all 2s ease">The long link</a>
```

In this bug, we encounter a distracting transition from 3px to 1px. The reason is that the default value of outline-width is Medium (3px), but we put a rule as follows in ua.css [1]:

```
*|*:any-link:-moz-focusring {
  /* Don't specify the outline-color, we should always use initial value. */
  outline: 1px dotted;
}
```

So, no matter we hit the chunk of code in [2] or not, we all set "outline: 1px dotted" for a focused link element, and the issue still remains. That's the reason why Cameron and I were discussing about the possibility of patching ua.css directly (the 2nd approach in comment 14).



[1] https://searchfox.org/mozilla-central/rev/cef8389c687203085dc6b52de2fbd0260d7495bf/layout/style/res/ua.css#128-131
[2] https://searchfox.org/mozilla-central/rev/
> cef8389c687203085dc6b52de2fbd0260d7495bf/layout/style/
> nsLayoutStylesheetCache.cpp#940-976

Comment 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

(In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> 1. Change the keyword mappings just for outline-width.
> To do so, we can map 'medium' to 1px, to avoid the transition from 3px to
> 1px. But, we will still see a transition from "outline-style: none;" to
> "outline-style: dotted;" (might be a flip transition effect I guess). Also,
> though we only change the keyword mappings for outline-width (keep the
> border-width as it is), there might be some consequences due to this change
> of mapping.

This breaks conformance to the specification.

> 2. Patch UA stylesheet for link's default style [1].
> To avoid the distracted transition, we can add "outline: transparent 1px
> dotted;" to link's default style. In this way, the transition would be
> nearly unobservable.

This only fixes the problem for links, and not for other things that are focusable with similar rules.  But fixing it for everything would essentially also be breaking conformance to the specification.


How do other browsers avoid the problem?  Or do they?

> 
> I personally a bit lean against the second approach, which IMO could avoid
> the weird dot transitioning totally.
> 
> Hi David, does the second approach make sense to you? I'd like to listen to
> your opinion before going further.
> 
> 
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 31311070d9860b24fe4a7a36976c14b328c16208/layout/style/res/ua.css#124-126

Comment 18

[Jeremy Chen ([e-mail])]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #17)
> (In reply to Jeremy Chen [:jeremychen] UTC+8 from comment #14)
> > 1. Change the keyword mappings just for outline-width.
> > To do so, we can map 'medium' to 1px, to avoid the transition from 3px to
> > 1px. But, we will still see a transition from "outline-style: none;" to
> > "outline-style: dotted;" (might be a flip transition effect I guess). Also,
> > though we only change the keyword mappings for outline-width (keep the
> > border-width as it is), there might be some consequences due to this change
> > of mapping.
> 
> This breaks conformance to the specification.

Yes, which is why I would not lean against this one.

> > 2. Patch UA stylesheet for link's default style [1].
> > To avoid the distracted transition, we can add "outline: transparent 1px
> > dotted;" to link's default style. In this way, the transition would be
> > nearly unobservable.
> 
> This only fixes the problem for links, and not for other things that are
> focusable with similar rules.  But fixing it for everything would
> essentially also be breaking conformance to the specification.
> 
> 
> How do other browsers avoid the problem?  Or do they?

You're right, this only fixes the problem for links. But, it is only the focused link element that we set this specific outline rule. So, we only need to fix the link element for now. We could put some comment to mention about the rule that we patch, and hint for people who might further want to add outline property to other focused element in future.

Chrome would also do the transitions as well, but not that distracting since they're not using "dotted" for the focus ring. I guess it is the 'solid' to 'dotted' transition that makes it more distracting.

Comment 19

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

So "lean against" means that you prefer to *not* choose that option.  I think you've been using it backwards.

I'd suggest just not worrying about this bug, leaving things as-is for now, and seeing if we get more reports.  It doesn't seem like there's a clear path to fixing it.

Comment 20

[Jeremy Chen ([e-mail])]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #19)
> So "lean against" means that you prefer to *not* choose that option.  I
> think you've been using it backwards.

Sorry about that.

> I'd suggest just not worrying about this bug, leaving things as-is for now,
> and seeing if we get more reports.  It doesn't seem like there's a clear
> path to fixing it.

Got it. Thanks for the feedback.
Lower the priority and un-assign myself accordingly.