Open Bug 1403483 Opened 3 years ago Updated 8 months ago

First tab separator should not span full height when drag space / menu bar is enabled

Categories

(Firefox :: Theme, defect, P3)

57 Branch
defect

Tracking

()

ASSIGNED
Tracking Status
firefox57 --- wontfix
firefox58 --- affected
firefox59 --- affected
firefox60 --- affected
firefox61 --- wontfix
firefox68 --- wontfix
firefox69 --- fix-optional
firefox70 --- affected

People

(Reporter: johannh, Assigned: nhnt11)

References

(Blocks 2 open bugs, Regression)

Details

(Whiteboard: [reserve-photon-visual])

Attachments

(4 files, 2 obsolete files)

See screenshot, the first tab separator should have the same height as the other separators. We forgot to do that in bug 1390025.
Flags: qe-verify+
QA Contact: ovidiu.boca
Let's do this after bug 1391539, otherwise we'll have to file another bug to adjust the height of this border when hovering (I'm happy to pick it up once bug 1391539 is done).
Depends on: 1391539
Duplicate of this bug: 1406681
Hi Johann,

I noticed that the overflow separator has also the bug.

Please see the screenshot. Thanks.
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
This works well for me at the moment on current central, but I need to confirm with Johann whether some changes around this code he was talking about have landed already or not. Also, I'm not entirely sure this works for RTL, I'm going to investigate this tomorrow.
Hmm, and I think I need to make the Windows-specific selector account for the menubar as well as maximized mode. Canceling review for now.
Attachment #8917126 - Flags: review?(dao+bmo)
Priority: P3 → P1
Blocks: 1408953
No longer blocks: 1408953
Duplicate of this bug: 1408953
Attachment #8917126 - Attachment is obsolete: true
Dão, the only problem (as far as I can tell) with this patch is that there is no tab separator before the first non-pinned tab when the tabstrip is overflowing. A CSS-only solution might be to use a selector like .tabbrowser-tab[pinned] + .tabbrowser-tab:not([pinned]), but I worry about hidden tabs in between. Do you think this would be an OK solution or should we try and add an attribute to the first visible non-pinned tab in JavaScript for styling convenience?
Flags: needinfo?(dao+bmo)
TODO: Add a rule to hide the first tab separator when sizemode != normal on Windows.
(In reply to Nihanth Subramanya [:nhnt11] from comment #13)
> TODO: Add a rule to hide the first tab separator when sizemode != normal on
> Windows.

Latest patch takes care of this. Need to investigate if Linux needs a similar rule.
Comment on attachment 8923047 [details]
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

https://reviewboard.mozilla.org/r/194262/#review200420

It's not clear to me why this patch moves the border away fron the pre-tabs whitespace. Why can we not just fix that border's height?
Attachment #8923047 - Flags: review?(dao+bmo)
Flags: needinfo?(dao+bmo)
(In reply to Dão Gottwald [::dao] from comment #16)
> Comment on attachment 8923047 [details]
> Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs
> whitespace.
> 
> https://reviewboard.mozilla.org/r/194262/#review200420
> 
> It's not clear to me why this patch moves the border away fron the pre-tabs
> whitespace. Why can we not just fix that border's height?

The border styling needs to change when the first tab is hovered/selected: is there an easy way to style the border of the pre-tabs whitespace depending on the :hover/visuallyselected state of the first tab? I don't see one.
Flags: needinfo?(dao+bmo)
In support of Nihanths current solution, it's probably what we need to do to resolve bug 1403520 as well.
I'm setting this to block bug 1403520 for now, as we probably shouldn't act there without solving this first.
Blocks: 1403520
Hey folks, is there anything we can do to resolve this anytime soon? I'm happy to take the review if you're busy right now, Dão.

Thanks!
Flags: needinfo?(nhnt11)
(In reply to Nihanth Subramanya [:nhnt11] from comment #21)
> Comment on attachment 8923047 [details]
> Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs
> whitespace.
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/194262/diff/4-5/

Rebased and added the sizemode != normal rule for Linux (re. comment 15 quoted below)

(In reply to Nihanth Subramanya [:nhnt11] from comment #15)
> (In reply to Nihanth Subramanya [:nhnt11] from comment #13)
> > TODO: Add a rule to hide the first tab separator when sizemode != normal on
> > Windows.
> 
> Latest patch takes care of this. Need to investigate if Linux needs a
> similar rule.

(In reply to Johann Hofmann [:johannh] from comment #20)
> Hey folks, is there anything we can do to resolve this anytime soon? I'm
> happy to take the review if you're busy right now, Dão.
> 
> Thanks!

Dão, re-flagged you for r?, let me know if I should redirect this to Johann.
Flags: needinfo?(nhnt11)
Blocks: 1439430
Attachment #8923047 - Attachment is obsolete: true
Attachment #8923047 - Flags: review?(dao+bmo)
Comment on attachment 8975470 [details]
Bug 1403483 - Show tab separator on first tab instead of on the pre-tabs whitespace.

https://reviewboard.mozilla.org/r/243752/#review253754

This is pretty fiddly to get right but I feel like you did a great job, although I'm not as immersed into this code as I was half a year ago :)

Generally this looks pretty good to me. I have one small complaint: I don't think the separator looks good on Windows 7 (non-maximized) like this. I can attach a screenshot. Considering that we're both not really working on tabs full-time anymore it's probably a good idea not to defer that to a follow-up bug.

We could consider trying to entirely hide the separator on Windows 7 (would need to check how that looks with light/dark theme, too). Let me know if you want to borrow my VM for that...
Attachment #8975470 - Flags: review?(jhofmann) → review-
Attached image Windows 7
Sorry for the small image, might have to zoom in.
Duplicate of this bug: 1464373
Flags: needinfo?(dao+bmo)

I see the same issue on the Mac, 10.14.3 with FF developer edition 67.0b3

Duplicate of this bug: 1538767
Blocks: 1567705
No longer blocks: 1567705
Duplicate of this bug: 1567705
Duplicate of this bug: 1567246

Is this still something we want to land? We have a couple of duplicates so I think it's an issue that bothers users and it's also marked P1.

(In reply to Liz Henry (:lizzard) (use needinfo) from comment #31)

Is this still something we want to land?

It's not ready to land, might not be even close.

Priority: P1 → P3
Flags: needinfo?(nhnt11)
You need to log in before you can comment on or make changes to this bug.