Closed Bug 1198703 Opened 6 years ago Closed 6 years ago

Separator color for combined/menu toolbar buttons should adjust based on the text color

Categories

(Firefox :: Theme, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 43
Tracking Status
firefox43 --- fixed

People

(Reporter: dao, Assigned: dao)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch patch (obsolete) — Splinter Review
We can have the separator color adjust based on the text color and drop the box-shadow (which was intended to ensure the separator is visible on dark backgrounds). This is similar to what we've done for the Firefox menu button separator or tab separators.
Attachment #8652817 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8652817 [details] [diff] [review]
patch

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

::: browser/themes/linux/browser.css
@@ +661,3 @@
>  }
>  
>  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before {

Is there a reason not to do this on the other toolbars, like on OS X?

Linux also seems to be missing the combined button versions of this stuff.

::: browser/themes/windows/browser.css
@@ +834,3 @@
>  
>  #nav-bar .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before,
>  #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled=true]) + .toolbarbutton-1:-moz-any(:not(:hover):not([open]),[disabled=true])::before {

Same question here, really.
Attachment #8652817 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8652817 [details] [diff] [review]
patch

(In reply to :Gijs Kruitbosch from comment #1)
> >  :-moz-any(#TabsToolbar, #nav-bar) .toolbarbutton-1:not(:hover):not(:active):not([open]) > .toolbarbutton-menubutton-dropmarker::before {
> 
> Is there a reason not to do this on the other toolbars, like on OS X?

On Linux we only use custom toolbar button styling on these toolbars. Otherwise we fall back on the default appearance.

> Linux also seems to be missing the combined button versions of this stuff.

Seems like it. Neither question really seems relevant for this bug, though... I'm not touching the selectors, just changing how we draw the background.
Attachment #8652817 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8652817 [details] [diff] [review]
patch

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

Tested on OS X, this makes the separators look too big: http://imgur.com/NETjh9Q

The bookmarks size is correct, the ones between cut/copy/paste and zoom out/level/in are wrong.
Attachment #8652817 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch patch v2 (obsolete) — Splinter Review
panelUIOverlay.inc.css had something to say about this as well *sigh*
Attachment #8652817 - Attachment is obsolete: true
Attachment #8652899 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8652899 [details] [diff] [review]
patch v2

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

On OSX, this makes the end-side ("second" in LTR) separator look off-center when fading in/out into the borders that show up on hover/active (when on the toolbar), because for whatever reason the vertical border for the 3-in-one items is on the center element, and the separator has negative end-margin.

The most straightforward solution is probably to make the border always be start-side, and give the last element in the combined button an end-border, assuming that doesn't break anything in the panel.
Attachment #8652899 - Flags: review?(gijskruitbosch+bugs) → review-
Attached patch patch v3Splinter Review
> The most straightforward solution is probably to make the border always be
> start-side, and give the last element in the combined button an end-border,
> assuming that doesn't break anything in the panel.

I'm having a hard time telling what kind of borders of what elements you're referring to here, but I've switched the separator from using -moz-margin-end back to -moz-margin-start on OS X. This should restore the status quo regarding the separators' position. If there's still something off without my patch having caused this, could you please file a new bug on it?
Attachment #8652899 - Attachment is obsolete: true
Attachment #8653336 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8653336 [details] [diff] [review]
patch v3

(In reply to Dão Gottwald [:dao] from comment #6)
> Created attachment 8653336 [details] [diff] [review]
> patch v3
> 
> > The most straightforward solution is probably to make the border always be
> > start-side, and give the last element in the combined button an end-border,
> > assuming that doesn't break anything in the panel.
> 
> I'm having a hard time telling what kind of borders of what elements you're
> referring to here, but I've switched the separator from using
> -moz-margin-end back to -moz-margin-start on OS X. This should restore the
> status quo regarding the separators' position. If there's still something
> off without my patch having caused this, could you please file a new bug on
> it?

The problem is that your patch does cause this. Maybe I can try to illustrate this visually. Take the cut/copy/paste buttons as an example:

------+------+--------
| cut | copy | paste |
------+------+--------

Now, the outer border is invisible, and the inner separators (between cut+copy, and between copy+paste) are smaller, when the items are not hovered:

  cut | copy | paste

Before your patch, those two separators (that are visible when not hovered) line up with the (larger) separators (that are visible when you do hover over the buttons).

After your previous patch, the separator to the right of "copy" no longer lined up when comparing the before/after hover state.

With this new patch, that is fixed but now the same happens to the separator to the left of "copy".

Because of the transition that fades these separators in/out, this is very noticeable.

The "full" borders (that show up on hover) are rendered by use of CSS borders on the buttons. The other ones are <separator>s.

I think either you need to change the border on the buttons, or you need to change the use of negative margin-start/end on the separator to depend on whether you're styling the first or second separator.

As it is, considering this is a regression, I don't think we should take this as-is.
Attachment #8653336 - Flags: review?(gijskruitbosch+bugs) → review-
(In reply to :Gijs Kruitbosch from comment #7)
> The problem is that your patch does cause this.

I'm still confused. What I'm doing on OS X is to adjust selectors, remove position:relative, top, background-image, background-clip, box-shadow and the panelUIOverlay.inc.css stuff and add background-image and opacity. Which of these changes would be relevant for what you're seeing?

(In reply to :Gijs Kruitbosch from comment #8)
> Created attachment 8653472 [details]
> Screen shot of misaligned border

Is this an actual screenshot or a mashup? The separators are supposed to disappear on hover...
Flags: needinfo?(gijskruitbosch+bugs)
Comment on attachment 8653336 [details] [diff] [review]
patch v3

(In reply to Dão Gottwald [:dao] from comment #9)
> (In reply to :Gijs Kruitbosch from comment #7)
> > The problem is that your patch does cause this.
> 
> I'm still confused. What I'm doing on OS X is to adjust selectors, remove
> position:relative, top, background-image, background-clip, box-shadow and
> the panelUIOverlay.inc.css stuff and add background-image and opacity. Which
> of these changes would be relevant for what you're seeing?

I'm sorry, I was wrong and the particular misalignment with the latest patch is identical to what was there before. I must have gotten confused about whether the patch was applied. :-\

FWIW, I assumed code-wise that the width: 3px change was responsible here, but it seems that that file is included before rather than after browser.css, which is a little surprising to me...

I'll file a followup bug.

> (In reply to :Gijs Kruitbosch from comment #8)
> > Created attachment 8653472 [details]
> > Screen shot of misaligned border
> 
> Is this an actual screenshot or a mashup? The separators are supposed to
> disappear on hover...

It's an actual screenshot; the borders (rather than separators) transition out over 250ms (which I increased in order to be able to time the screenshot more easily) when you stop hovering.
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8653336 - Flags: review- → review+
https://hg.mozilla.org/mozilla-central/rev/2ae8fcb73ae6
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
No longer depends on: 1202122
Depends on: 1317686
Depends on: 1340436
No longer depends on: 1317686
You need to log in before you can comment on or make changes to this bug.