Closed Bug 981700 Opened 6 years ago Closed 6 years ago

Australis, toolbarbutton type="checkbox" doesn't have a visible checked state.

Categories

(Firefox :: Theme, defect)

x86
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox29 --- verified
firefox30 --- verified

People

(Reporter: zer0, Assigned: mikedeboer)

References

Details

(Whiteboard: [Australis:P3+])

Attachments

(1 file, 3 obsolete files)

Not sure if it's limited to OS X only, but currently `<toolbarbutton type="checkbox" checked="true">` doesn't show any visible state, therefore is broken.

I believe the state should be probably the same of bug 909349, but maybe could be worthy double check with the other platform (Windows, Linux) or UX.
Blocks: australis
Depends on: 909349
Whiteboard: [Australis:P3+]
(this already works on Windows, I just checked)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Comment on attachment 8389325 [details] [diff] [review]
Patch v1: introduce a checked state for toolbar buttons with type=checkbox

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

On both OS X and Linux if you hover over a checked item, no state change happens, while it does happen on Windows (ie, you see the :hover style, not the :active/[checked] one). I think this makes sense because the user should have a sense they can uncheck/press the button. We should do the same on Linux and OS X.
Attachment #8389325 - Flags: review?(gijskruitbosch+bugs) → feedback+
I'm not so sure about that, it's already quite different that we have a hover state on OSX and a hover state for checked items seems even more alien to me.
Stephen, what do you think?

Linux, sure. It mirrors the Windows theme already, so we should be consistent here too.
Flags: needinfo?(shorlander)
Over IRC Stephen gave me the style to use for the hover color on OSX.
Attachment #8389852 - Attachment is obsolete: true
Attachment #8390127 - Flags: review?(gijskruitbosch+bugs)
Flags: needinfo?(shorlander)
Attachment #8390127 - Attachment is obsolete: true
Attachment #8390127 - Flags: review?(gijskruitbosch+bugs)
Attachment #8390148 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8390148 [details] [diff] [review]
Patch v1.3: introduce a checked state for toolbar buttons with type=checkbox

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

Haven't tested this, so I hope you did! Generally, looks non-crazy, although you forgot the transition timing update we discussed on IRC in the linux hunk.

What I'm genuinely confused about is why we can't just add the selector you added on linux to the other :hover selectors. That's what worked on mac, right? Is there some reason that doesn't work here? That'd be simpler.

r+ either way

::: browser/themes/linux/browser.css
@@ +624,5 @@
>    transition-duration: 10ms;
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1[checked]:not(:active):hover > .toolbarbutton-icon {
> +  background-color: rgba(90%,90%,90%,.4);

What's with the magic number? I don't think this is what Stephen mentioned on IRC (that was the mac bit, right?). Also, we seem to use ints for rgba... 231? Can't we just add this selector to the 'normal' :hover style?

@@ +625,5 @@
>  }
>  
> +:-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1[checked]:not(:active):hover > .toolbarbutton-icon {
> +  background-color: rgba(90%,90%,90%,.4);
> +  transition: background-color .4s;

Nit: ITYM 250ms ?
Attachment #8390148 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs Kruitbosch from comment #8)
> What's with the magic number? I don't think this is what Stephen mentioned
> on IRC (that was the mac bit, right?). Also, we seem to use ints for rgba...
> 231? Can't we just add this selector to the 'normal' :hover style?

This is an exact copy of how the Windows theme is doing it. We're currently matching the Windows theme as much as possible re. the toolbar styling, so having this copied over fits.

> Nit: ITYM 250ms ?

150ms, actually :) Transition-durations are different on each platform. Why? No clue!
Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/cc1fdefca201
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
This landed with the wrong bug number.

https://hg.mozilla.org/mozilla-central/rev/cc1fdefca201
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Target Milestone: --- → Firefox 30
Comment on attachment 8390148 [details] [diff] [review]
Patch v1.3: introduce a checked state for toolbar buttons with type=checkbox

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: Buttons of type 'checkbox' will not have a visible checked state on OSX and Linux without this patch applied.
Testing completed (on m-c, etc.): landed on m-c.
Risk to taking this patch (and alternatives if risky): minor
String or IDL/UUID changes made by this patch: n/a
Attachment #8390148 - Flags: approval-mozilla-aurora?
Attachment #8390148 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.