Border effect for tab overflow

RESOLVED FIXED in Firefox 4.0b12

Status

()

Firefox
Theme
RESOLVED FIXED
7 years ago
4 years ago

People

(Reporter: faaborg, Assigned: fryn)

Tracking

({regression})

Trunk
Firefox 4.0b12
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [target-betaN][hardblocker])

Attachments

(3 attachments, 7 obsolete attachments)

This bug is a subset of:

Bug 570564 - Improve tab overflow navigation
and 
Bug 579728 - when scrolling the tab container, show half of the first overflow tab in the scroll direction

the goal here is just to land the shadow being drawn on the overflowed tabs, shown here:

https://bug570564.bugzilla.mozilla.org/attachment.cgi?id=449696
(Assignee)

Comment 1

7 years ago
I've done this with CSS box-shadow hacks, but it looks really messy.

An image would be better. CC'ing shorlander for help with that.
Keywords: icon
Version: unspecified → Trunk
Whiteboard: [target-betaN]
(Assignee)

Updated

7 years ago
Blocks: 570564

Comment 2

7 years ago
Created attachment 504049 [details]
overflow button (hovered) is kind of wrong

Can we fix this overflow button hover state while we're at it?
Duplicate of this bug: 626755
This is a theme regression since the separator was removed from the tab bar buttons earlier. I believe this is serious enough to be a soft blocker, as it just looks plain broken right now.
blocking2.0: --- → ?
Keywords: regression
blocking2.0: ? → final+
Whiteboard: [target-betaN] → [target-betaN][softblocker]
I actually think this might be a hardblocker ... looks *awful* on Windows 7.
Whiteboard: [target-betaN][softblocker] → [target-betaN][hardblocker][?]
(Assignee)

Updated

7 years ago
Assignee: nobody → fryn
Status: NEW → ASSIGNED
Created attachment 506869 [details]
Mockup: Divider line current design vs intended design on OSX
Attachment #506869 - Attachment description: Mockup: Mockup: Divider line current design vs intended design on OSX → Mockup: Divider line current design vs intended design on OSX
Created attachment 507117 [details] [diff] [review]
WIP Tab Overflow Patch - 01

I took a shot at this using -moz-border-image and setting a negative margin on the scrollbuttons.
Attachment #507117 - Flags: feedback?(fryn)
Created attachment 507119 [details]
Screenshot
(Assignee)

Comment 9

7 years ago
Comment on attachment 507117 [details] [diff] [review]
WIP Tab Overflow Patch - 01

IIRC, you mentioned that this breaks app tab positioning.
The shadow should extend to the bottom border of the tab strip, but it doesn't seem to do that on the right side in the screenshot.
Also, the shadow should ignore mouse events.

It's a good start. I'll see what I can do to improve upon it.
Thanks for pitching in. :)
Attachment #507117 - Flags: feedback?(fryn) → feedback-
(In reply to comment #9)
> IIRC, you mentioned that this breaks app tab positioning.

The "margin-left: -6px" moved the apptabs over 6px as well. Adding a balancing "margin-right: 6px" fixed this and still positioned the tabs behind the shadow. However I can't claim to understand the ramifications of doing such a thing :)

> Also, the shadow should ignore mouse events.

Yes. This method won't work for that. As we discussed an additional non-interactive element might work.

> It's a good start. I'll see what I can do to improve upon it.

Thank you!
Whiteboard: [target-betaN][hardblocker][?] → [target-betaN][hardblocker][?][needs new patch]
The attached patch seems to miss tab-overflow-shadow.png.
(In reply to comment #11)
> The attached patch seems to miss tab-overflow-shadow.png.

It doesn't. Sorry for the noise.
Frank - ETA for a new patch, here?
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> Frank - ETA for a new patch, here?

We ran into problems trying to get the shadow effect without invasive surgery, so we're scoping this down to a border effect, since that's the hard-blocking part anyway.

ETA: tonight.
Summary: Shadow effect for tab overflow → Border effect for tab overflow
(Assignee)

Updated

7 years ago
Blocks: 632156
(Assignee)

Comment 15

7 years ago
Created attachment 510442 [details] [diff] [review]
patch

Filed bug 632156 for the shadow effect followup.

(In reply to comment #2)
> Created attachment 504049 [details]
> overflow button (hovered) is kind of wrong

This patch mitigates the problem, but it's still there. It's non-trivial to make small tweaks to native styling. We may decide to do non-native styling on the buttons to circumvent this, but that will be in a followup.
Attachment #506869 - Attachment is obsolete: true
Attachment #507117 - Attachment is obsolete: true
Attachment #507119 - Attachment is obsolete: true
Attachment #510442 - Flags: ui-review?(shorlander)
Attachment #510442 - Flags: review?(dao)
(Assignee)

Updated

7 years ago
Keywords: icon
Whiteboard: [target-betaN][hardblocker][?][needs new patch] → [target-betaN][hardblocker]
Comment on attachment 510442 [details] [diff] [review]
patch

Taking the review from shorlander to accelerate us landing this.  I checked classic, basic, glass and panorama on fryn's machine.  Overall looked good, no shadow but it kind of looks like there is one, due to the gradient on tabs (our mental hardware seems to kind of fill in the shadow for us).
Attachment #510442 - Flags: ui-review?(shorlander) → ui-review+

Updated

7 years ago
Whiteboard: [target-betaN][hardblocker] → [target-betaN][hardblocker][has patch]
Whiteboard: [target-betaN][hardblocker][has patch] → [target-betaN][hardblocker][has patch][needs review dao]
Comment on attachment 510442 [details] [diff] [review]
patch

Only tested this on Linux right now. The custom hover effect looks bogus, I think we can drop that without replacement (there's no hover effect without this patch either).

Windows and Mac are next up.

>+.tabbrowser-arrowscrollbox > .scrollbutton-up:not([disabled="true"]):-moz-locale-dir(ltr),
>+.tabbrowser-arrowscrollbox > .scrollbutton-down:not([disabled="true"]):-moz-locale-dir(rtl) {

nit: just [disabled] will do
Attachment #510442 - Flags: review?(dao) → review-
(Assignee)

Comment 18

7 years ago
(In reply to comment #17)

Thanks for the Linux review; I will address those asap. Also, shorlander noted that all these need to work in tabs-on-bottom mode. I'll also adjust the patch for that.

I put up tryserver builds here with the above patch:
https://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/fyan@mozilla.com-dbddb463e8a2/
The rest of the patch seems fine. Tabs on bottom on Mac also works surprisingly well.
Whiteboard: [target-betaN][hardblocker][has patch][needs review dao] → [target-betaN][hardblocker][needs new patch]
(Assignee)

Comment 20

7 years ago
Created attachment 510931 [details] [diff] [review]
patch v2

Removed linux hover effect and fixed nit as per comment 17.

Fixed tabs-on-bottom mode on OS X.
Attachment #510442 - Attachment is obsolete: true
Attachment #510931 - Flags: review?(dao)
Whiteboard: [target-betaN][hardblocker][needs new patch] → [target-betaN][hardblocker][has patch][needs review dao]
(Assignee)

Comment 21

7 years ago
Created attachment 510934 [details] [diff] [review]
patch v3

Oops, missed a couple [disabled="true"]'s. No change in patch behavior.
Attachment #510931 - Attachment is obsolete: true
Attachment #510934 - Flags: review?(dao)
Attachment #510931 - Flags: review?(dao)
Comment on attachment 510934 [details] [diff] [review]
patch v3

>--- a/browser/themes/pinstripe/browser/browser.css
>+++ b/browser/themes/pinstripe/browser/browser.css

>+#tabbrowser-tabs[tabsontop="false"] > .tabbrowser-arrowscrollbox > .scrollbutton-up,
>+#tabbrowser-tabs[tabsontop="false"] > .tabbrowser-arrowscrollbox > .scrollbutton-down {
>+  -moz-transform: scaleY(-1);
>+}

I think you need to flip the buttons' icons a second time for this to produce the right result.
Attachment #510934 - Flags: review?(dao) → review-
(Assignee)

Comment 23

7 years ago
Created attachment 511018 [details] [diff] [review]
patch v4

(In reply to comment #22)

The buttons' icons are almost completely symmetrical across the horizontal axis, so I didn't notice it, but it's addressed now.
Attachment #510934 - Attachment is obsolete: true
Attachment #511018 - Flags: review?(dao)

Updated

7 years ago
Attachment #511018 - Flags: review?(dao) → review+
(Assignee)

Updated

7 years ago
Whiteboard: [target-betaN][hardblocker][has patch][needs review dao] → [target-betaN][hardblocker]

Updated

7 years ago
Whiteboard: [target-betaN][hardblocker] → [target-betaN][hardblocker][has patch]
Whiteboard: [target-betaN][hardblocker][has patch] → [target-betaN][hardblocker][has patch][can land]
(Assignee)

Comment 24

7 years ago
Created attachment 511096 [details] [diff] [review]
hg export for patch v4 [r=dao ui-r=faaborg]

Margaret kindly offered to land this later today. Thanks! :)
Attachment #511018 - Attachment is obsolete: true
Landed: http://hg.mozilla.org/mozilla-central/rev/d37fedadd4ec
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b12

Updated

7 years ago
Blocks: 578162

Comment 26

7 years ago
Created attachment 511380 [details]
Blue overflow button upon startup

Regression after landing (please check attachment). When browser session is restored, blue overflow button is visible for a few moment.
(Assignee)

Updated

7 years ago
Whiteboard: [target-betaN][hardblocker][has patch][can land] → [target-betaN][hardblocker]

Comment 27

7 years ago
This bug seems to have triggered the return of bug 624157. Should I file a new bug or should that bug be reopened?
(In reply to comment #27)
> This bug seems to have triggered the return of bug 624157. Should I file a new
> bug or should that bug be reopened?

You should file a new bug.

Updated

7 years ago
Depends on: 633524

Updated

7 years ago
No longer depends on: 633524
(Assignee)

Updated

7 years ago
Blocks: 633896
You need to log in before you can comment on or make changes to this bug.