Badged menu-buttons on Windows don't highlight

NEW
Assigned to

Status

()

Firefox
Toolbars and Customization
3 years ago
3 years ago

People

(Reporter: mkaply, Assigned: mkaply)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox42 affected)

Details

Attachments

(2 attachments)

(Assignee)

Description

3 years ago
If you create a badged menu-button, hovering over it doesn't show the highlight.

I have a patch for this.

Comment 1

3 years ago
> I have a patch for this.

That's nice. Can we have it, too? ;-)
Assignee: nobody → mozilla
(Assignee)

Comment 2

3 years ago
Created attachment 8627367 [details] [diff] [review]
Add in missing cases

(In reply to Ben Bucksch (:BenB) from comment #1)
> > I have a patch for this.
> 
> That's nice. Can we have it, too? ;-)

Where do you think the patch came from? :)


This adds in the missing cases. Basically anywhere there was a .toolbarbutton-menubutton-button > .toolbarbutton-icon, it should have had a .toolbarbutton-menubutton-button > .toolbarbutton-badge-container as well.
Attachment #8627367 - Flags: review?(gijskruitbosch+bugs)

Comment 3

3 years ago
I'm sorry for not reviewing this within my normal deadline of 24 hours, I'm still getting out of whistler backlog (and having to deal with MS releasing win10 updates all the time).

Do you have an add-on that I can test this with? The CSS looks fine, but I'd like to actually see it apply to something, and I don't have usable badged menu buttons.
Flags: needinfo?(mozilla)

Comment 4

3 years ago
Comment on attachment 8627367 [details] [diff] [review]
Add in missing cases

It's hard to be sure without a testcase, but I expect this will also need changes in panelUIOverlay.(inc.)css , and maybe devedition CSS to not break stuff just in that theme.
Attachment #8627367 - Flags: review?(gijskruitbosch+bugs)
(Assignee)

Comment 5

3 years ago
> Do you have an add-on that I can test this with? The CSS looks fine, but I'd like to actually see it apply to something, and I don't have usable badged menu buttons.

I'll get a test add-on together.
Flags: needinfo?(mozilla)
(Assignee)

Comment 6

3 years ago
Created attachment 8628285 [details]
Test XPI

Here's an XPI to test with.

It adds a toolbar and nav buttons to compare.

Underneath the menus are menuitems that let you remove the badges to compare.

Not sure if you are using HiRes Windows or not, but there is some weirdness around the icons on the nav bar disappearing when you turn off the badges. Investigating that as a separate issue.
(Assignee)

Comment 7

3 years ago
And this only works on nightly. For some reason on FF38, adding badges to any button with  menus makes the dropmarker disappear? I'm at a loss.

Comment 8

3 years ago
(In reply to Mike Kaply [:mkaply] from comment #7)
> And this only works on nightly. For some reason on FF38, adding badges to
> any button with  menus makes the dropmarker disappear? I'm at a loss.

Didn't the badged-menubutton support not land until quite recently, so it's not in Fx38? bug 1157688 is marked fixed in 40, but not 39 or 38, so that makes sense to me...

(In reply to :Gijs Kruitbosch from comment #4)
> It's hard to be sure without a testcase, but I expect this will also need
> changes in panelUIOverlay.(inc.)css , and maybe devedition CSS to not break
> stuff just in that theme.

Were you planning on changing the patch to address this feedback?
Flags: needinfo?(mozilla)

Comment 9

3 years ago
(In reply to :Gijs Kruitbosch from comment #8)
> (In reply to :Gijs Kruitbosch from comment #4)
> > It's hard to be sure without a testcase, but I expect this will also need
> > changes in panelUIOverlay.(inc.)css , and maybe devedition CSS to not break
> > stuff just in that theme.
> 
> Were you planning on changing the patch to address this feedback?

(just so we're clear, not trying to push, just wondering if I need to re-review right now or wait)
(Assignee)

Comment 10

3 years ago
> Didn't the badged-menubutton support not land until quite recently, so it's not in Fx38? bug 1157688 is marked fixed in 40, but not 39 or 38, so that makes sense to me...

Yes, but even regular menu buttons get their dropmarkers dropped. It's weird.

> Were you planning on changing the patch to address this feedback?

Yes. Let me see if anything else is needed before reviewing.

tx
Flags: needinfo?(mozilla)

Comment 11

3 years ago
(In reply to Mike Kaply [:mkaply] from comment #10)
> > Didn't the badged-menubutton support not land until quite recently, so it's not in Fx38? bug 1157688 is marked fixed in 40, but not 39 or 38, so that makes sense to me...
> 
> Yes, but even regular menu buttons get their dropmarkers dropped. It's weird.

Err, that's a regression, I'm 99% sure - we did quite a bit of work to make that work for Australis... Can you file a separate bug and needinfo me? (bonus points if you have an easy testcase for a non-badged menu button as well...)

> > Were you planning on changing the patch to address this feedback?
> 
> Yes. Let me see if anything else is needed before reviewing.

Perfect, thanks!

Comment 12

3 years ago
bug 1029937 is going to bitrot things here, but it might help you a bit. Hopefully.
(Assignee)

Comment 13

3 years ago
(In reply to :Gijs Kruitbosch from comment #12)
> bug 1029937 is going to bitrot things here, but it might help you a bit.
> Hopefully.

Yeah, I'm quite happy with Neil's changes. Should make things a little better.
You need to log in before you can comment on or make changes to this bug.