1576363 - Dialog default button is in active color even if the button gets unfocused

Status: VERIFIED FIXED

(Core :: XUL, defect, P2)

Description

[Alice0775 White]

Attachment: image.png

Reproducible: always

Steps to reproduce:

1. Open any dialog such as
   Choose User Profile
   Clear All History
   Download helper dialog etc...
2. Keypress [Tab] key several times to move focus to non-default button

Actual Results:
The default button has always in blue border

Expected Results:
Blue border should become gray when unfocused

Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=5e7c2d9420574ed055721a9685ff0d048d15399c&tochange=843c150636f81fb2f13196844206514f8a938bd9

Regressed by:
843c150636f81fb2f13196844206514f8a938bd9 Alexander Surkov — Bug 1544916 - migrate dialog binding to Custom Element r=bgrins,whimboo

Comment 1

[alexander :surkov (:asurkov)]

Cannot reproduce on OS X with Clear All History dialog, I see the issue on Windows build. Surprisingly, the Inspector doesn't show any relevant border or outline styles on a default button. When I remove @default="true" attribute from the button, the style disappears. I checked the code, but couldn't find anything relevant [1].

Any ideas, where those style are coming from?

[1] https://searchfox.org/mozilla-central/search?q=%5Bdefault&case=false&regexp=false&path=*.css

Comment 2

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #1)

Cannot reproduce on OS X with Clear All History dialog, I see the issue on Windows build. Surprisingly, the Inspector doesn't show any relevant border or outline styles on a default button. When I remove @default="true" attribute from the button, the style disappears. I checked the code, but couldn't find anything relevant [1].

Any ideas, where those style are coming from?

[1] https://searchfox.org/mozilla-central/search?q=%5Bdefault&case=false&regexp=false&path=*.css

another one observation: I cannot change border style on the button via devtools, it just doesn't have any effect. I'm curious, if there's something wrong with styling engine here? Emilio, do you have thoughts on this?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

I don't have a windows machine handy, but it seems unlikely to be a bug in the style engine... Does getComputedStyle() return what you set?

Anyhow, I see some manual state management in nsButtonBoxFrame, does the button incorrectly report true button.matches(":hover") or active?

Any way to repro this on Linux?

Comment 4

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

Too late for a fix in 70.

Comment 5

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

Brian can you help assign a priority or find an owner for this bug? Thanks!

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #3)

I don't have a windows machine handy, but it seems unlikely to be a bug in the style engine... Does getComputedStyle() return what you set?

Anyhow, I see some manual state management in nsButtonBoxFrame, does the button incorrectly report true button.matches(":hover") or active?

Any way to repro this on Linux?

Alex, can you check this please?

Comment 7

[alexander :surkov (:asurkov)]

I don't have linux at hands to check it out, but I suppose this is Windows only issue.

So CSS border changes are indeed reflected in getComputedStyle(), but have no visual effect, which apparently because of -moz-appearance:button style applied. When the style is off, then border is changed just nicely. Looking into Windows theme code these lines look relevant (https://searchfox.org/mozilla-central/source/widget/windows/nsNativeThemeWin.cpp#861-863):

  // Check for default dialog buttons.  These buttons should always look
  // focused.

It seems like a default button is always supposed look focused on Windows, which contradicts with the bug report. This code wasn't touched for a while, so if it's indeed a culprit of the behavior, then not sure what kept it sleeping all the time until bug 1544916 has landed.

Emilio, any thoughts on these findings?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

So this is totally web-observable then, right? Just checked in browserstack and indeed we paint a focused button for data:text/html,<button default>Foo</button>.

No other browser (checked Chrome and Edge also in BrowserStack) does it. So we should remove that code IMO. Worth checking why it didn't reproduce before... Did the buttons have a different appearance somehow?

But yeah +1 from me to get rid of that condition.

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #8)

So this is totally web-observable then, right? Just checked in browserstack and indeed we paint a focused button for data:text/html,<button default>Foo</button>.

No other browser (checked Chrome and Edge also in BrowserStack) does it. So we should remove that code IMO.

agreed

Worth checking why it didn't reproduce before... Did the buttons have a different appearance somehow?

nah, we were used to set "default" attribute off [1], so apparently this code [2] doesn't longer work. Having said that, if we are going to match Chrome/Edge, then let's just remove this piece of code.

[1] https://hg.mozilla.org/mozilla-central/annotate/776c731d7c71a3a9845671c624c8335d5533dc81/toolkit/content/widgets/dialog.xml#l478
[2] https://searchfox.org/mozilla-central/source/toolkit/content/widgets/dialog.js#60

Comment 10

[alexander :surkov (:asurkov)]

fyi, this unset default focus logic is from beginning of times (CVS Gecko 1.9/Firefox 3), so luck to bring any light at the reasoning of the behavior (https://hg.mozilla.org/mozilla-central/annotate/7bbd1a09eacb8a31f4caef5c1e526d8d32569ae9/toolkit/content/widgets/dialog.xml).

Comment 11

[alexander :surkov (:asurkov)]

Attachment: Bug 1576363 - Dialog default button is in active color even if the button gets unfocused

Comment 12

[alexander :surkov (:asurkov)]

if focused bit is removed from nsNatveThemeWin.cpp then all dialog buttons look same, which is weird, since there's no indication of a default button. It sounds like UX issue.

Comment 13

[alexander :surkov (:asurkov)]

Attachment: Bug 1576363 - Unset active styling for the default dialog button when another button inside the dialog gets focused

Comment 14

[alexander :surkov (:asurkov)]

Filed patch D49952 which restores original behavior. I.e. the 'ok' button looks default/focused when other elements in the dialog are focused, and looks nondefault/nonfocused when the cancel button gets focused: this behavior I observe in esr68.

I do see a styling glitch though: when I focus out the cancel button and thus make 'ok' button default again, the blue border around the 'ok' button doesn't get rendered in whole. I suppose it's a different issue though, so it'd be good to confirm it after D49952 is landed, and file a bug if it's not my machine glitch.

Also let's leave button@default out of scope of this bug: agreed we should remove @default attribute on HTML buttons for parity with other browsers, however it apperas @default makes perfect sense for XUL dialogs/buttons. I will file a followup for HTML:buttons.

Comment 15

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #14)

Also let's leave button@default out of scope of this bug: agreed we should remove @default attribute on HTML buttons for parity with other browsers, however it apperas @default makes perfect sense for XUL dialogs/buttons. I will file a followup for HTML:buttons.

filed bug 1590142 for HTML buttons

Comment 16

[Mats Palmgren (inactive)]

FYI, in native UI, a default button in a dialog is a different concept from the currently focused button.
https://devblogs.microsoft.com/oldnewthing/20040802-00/?p=38283

Rendering it the same as a focused button is obviously incorrect.
IIRC, on Linux/Unix desktop environments it's usually rendered with an extra border (so it would have a double border).

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Mats Palmgren (:mats) from comment #16)

FYI, in native UI, a default button in a dialog is a different concept from the currently focused button.
https://devblogs.microsoft.com/oldnewthing/20040802-00/?p=38283

Rendering it the same as a focused button is obviously incorrect.
IIRC, on Linux/Unix desktop environments it's usually rendered with an extra border (so it would have a double border).

I think there's misusing of the focused term in this bug, I think what the bug summary says is the default button in dialogs is always default (which, granted, look close to focused, however default focused button can be still distinguished from default not focused button by a blue outline around it); however it wasn't always used to be this way: prior to bug 1544916, the dialog had unset default attribute for a default button when focus went to other dialog buttons.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/118d8c49d63f
Unset active styling for the default dialog button when another button inside the dialog gets focused r=bgrins

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/118d8c49d63f

Comment 20

[Pascal Chevrel:pascalc (PTO until April 26)]

Alexander, is it worth uplifting this to 71 beta or should it ride the 72 train? Thanks.

Comment 21

[Pascal Chevrel:pascalc (PTO until April 26)]

(In reply to Pascal Chevrel:pascalc from comment #20)

Alexander, is it worth uplifting this to 71 beta or should it ride the 72 train? Thanks.

Emilio, could you handle my request while Alexander is on PTO please? Thanks

Comment 22

[Emilio Cobos Álvarez (:emilio)]

I think Brian would be better suited to assess this, I don't know how many of these dialogs we have.

Comment 23

[Brian Grinstead [:bgrins]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #22)

I think Brian would be better suited to assess this, I don't know how many of these dialogs we have.

Alex is back later this week - I'd like to wait for his input.

Comment 24

[alexander :surkov (:asurkov)]

Comment on attachment 9103021 [details]
Bug 1576363 - Unset active styling for the default dialog button when another button inside the dialog gets focused

Beta/Release Uplift Approval Request

• User impact if declined: confusing default button appearance on Windows in dialogs, for example, in Clear History dialog
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: see comment #0
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): relatively small change, replicating behavior we used to have in XBL dialogs
• String changes made/needed: no

Comment 25

[alexander :surkov (:asurkov)]

(In reply to Pascal Chevrel:pascalc from comment #20)

Alexander, is it worth uplifting this to 71 beta or should it ride the 72 train? Thanks.

It makes sense, dialogs look rather confusing on Windows, the patch doesn't look harmful.

Comment 26

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9103021 [details]
Bug 1576363 - Unset active styling for the default dialog button when another button inside the dialog gets focused

Low risk, patch was on nightly for 2 weeks with no reported regression, uplift approved for 71 beta 8, thanks.

Comment 27

[Daniel Varga [:dvarga]]

asurkow: please update the patches for beta, I'm a conflict when I tried to uplift.
First I applied the patch D49425 after that I grafted the central revision.
The conflict was for file toolkit/content/widgets/dialog.js

Comment 28

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9103021 [details]
Bug 1576363 - Unset active styling for the default dialog button when another button inside the dialog gets focused

Let's wait for alexander's info before we take this uplift.

Comment 29

[ovidiu boca[:Ovidiu]]

I verified this issue on Windows 10 with FF Nightly 72.0a1(2019-11-07) and I can confirm the fix.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to Daniel Varga [:dvarga] from comment #27)

asurkow: please update the patches for beta, I'm a conflict when I tried to uplift.
First I applied the patch D49425 after that I grafted the central revision.
The conflict was for file toolkit/content/widgets/dialog.js

nah, you only need D49952, D49425 never hit central

Comment 31

[Tim Nguyen :ntim]

Comment on attachment 9101561 [details]
Bug 1576363 - Dialog default button is in active color even if the button gets unfocused

Clearing based on comment 30

Comment 32

[Pascal Chevrel:pascalc (PTO until April 26)]

Comment on attachment 9103021 [details]
Bug 1576363 - Unset active styling for the default dialog button when another button inside the dialog gets focused

Uplift approved for 71 beta 9, thanks.

Comment 33

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/6ef29e4dcdf5

Comment 34

[ovidiu boca[:Ovidiu]]

I verified this issue on Windows 10 with FF 71.0b9 and I can confirm the fix.