Closed
Bug 875180
Opened 12 years ago
Closed 11 years ago
When a tab scrolls into view, half of the curve remains out of view
Categories
(Firefox :: Theme, defect)
Firefox
Theme
Tracking
()
RESOLVED
FIXED
Firefox 28
People
(Reporter: MattN, Assigned: jaws)
References
(Depends on 1 open bug)
Details
(Whiteboard: [Australis:M9][Australis:P2])
Attachments
(2 files, 1 obsolete file)
|
(deleted),
image/png
|
Details | |
|
(deleted),
patch
|
Unfocused
:
review+
|
Details | Diff | Splinter Review |
Screenshot with half of the tab curve hidden
When you have overflow and a tab scrolls into view, the entire tab should be visible, not just the middle (.tab-background-middle). Currently, the half (15px) of the curve which overlaps the adjacent tab remains hidden in the regions scrolled out of view.
STR:
1) Open many tabs so there is overflow
2) Select the 3rd last tab
3) Scroll the selected tab out of view
4) Switch to the 2nd last tab with the keyboard (Ctrl-tab)
Expected result:
The tab scrolls into view
Actual result:
~15px of the selected tab curve remains hidden
This is likely because of the overlap between tabs implemented with negative margins on tab elements. Look for usage of tabCurveHalfWidth in the CSS.
|
||
Updated•12 years ago
|
Whiteboard: [Australis:M?] → [Australis:M6]
|
||
Updated•12 years ago
|
Assignee: nobody → mnoorenberghe+bmo
|
|
||
Updated•12 years ago
|
Whiteboard: [Australis:M6] → [Australis:M7]
|
|
||
Updated•12 years ago
|
Whiteboard: [Australis:M7] → [Australis:M7][User Research Build+]
|
|
||
Comment 2•11 years ago
|
||
These bugs didn't make it into the UR Build that went out in bug 879846. Clearing the [User Research Build+] flag.
Whiteboard: [Australis:M7][User Research Build+] → [Australis:M7]
|
|
||
Comment 3•11 years ago
|
||
Removing the items from M7 that do not block landing on m-c.
Whiteboard: [Australis:M7] → [Australis:M?]
|
||
Comment 4•11 years ago
|
||
This has been brought up a few times as a major visual glitch, so P2.
Whiteboard: [Australis:M?] → [Australis:M?][Australis:P2]
|
|
||
Comment 6•11 years ago
|
||
(somewhat related to bug 579728, as Matt pointed out to me a while ago...)
|
Reporter | |
Updated•11 years ago
|
|
Assignee | |
Comment 7•11 years ago
|
||
Assignee: mnoorenberghe+bmo → jaws
Status: NEW → ASSIGNED
Attachment #819209 -
Flags: review?(gijskruitbosch+bugs)
|
|
||
Comment 8•11 years ago
|
||
Comment on attachment 819209 [details] [diff] [review]
Patch
Review of attachment 819209 [details] [diff] [review]:
-----------------------------------------------------------------
Clearing review because of the caching and toolkit-reviewer issue. If we can just cache these, IMO we should do that, for sure, before thinking about landing this. I'd still like to get bug 579728 done, but if that turns out to take too long I'm OK with going this route to just stick to what we already have on m-c, and at least not make things worse.
::: browser/base/content/tabbrowser.xml
@@ +3244,5 @@
> + <body><![CDATA[
> + let tabMiddle = document.getAnonymousElementByAttribute(aTab, "class", "tab-background-middle");
> + let tabMiddleStyle = window.getComputedStyle(tabMiddle, null);
> + let tabMarginLeft = parseFloat(tabMiddleStyle.marginLeft);
> + let tabMarginRight = parseFloat(tabMiddleStyle.marginRight);
AFAIK in our implementation these are always the same for all tabs, and this causes sync reflows. Seems to me like we should find a better way of getting these and caching them, not computing them every time.
::: toolkit/content/widgets/scrollbox.xml
@@ +225,5 @@
> + if (this._adjustElementStartAndEnd) {
> + [elementStart, elementEnd] =
> + this._adjustElementStartAndEnd(element, elementStart, elementEnd);
> + }
> +
I'm not a toolkit peer, and I'm not sure we should be modifying scrollbox.xml for a styling issue in the tabbrowser usage of it. Can you get review from a toolkit peer? :-)
Attachment #819209 -
Flags: review?(gijskruitbosch+bugs)
|
|
Assignee | |
Comment 9•11 years ago
|
||
Sounds good, yeah this should get a toolkit review.
Attachment #819209 -
Attachment is obsolete: true
Attachment #819222 -
Flags: review?(dao)
|
|
Assignee | |
Updated•11 years ago
|
Attachment #819222 -
Flags: review?(bmcbride)
|
||
Comment 10•11 years ago
|
||
Comment on attachment 819222 [details] [diff] [review]
Patch v1.1
Review of attachment 819222 [details] [diff] [review]:
-----------------------------------------------------------------
r+ for toolkit change, but add a comment stating the function is expected to come from an inherited binding.
Attachment #819222 -
Flags: review?(bmcbride) → review+
|
|
||
Comment 11•11 years ago
|
||
Comment on attachment 819222 [details] [diff] [review]
Patch v1.1
Review of attachment 819222 [details] [diff] [review]:
-----------------------------------------------------------------
And r+ for the browser changes too.
Attachment #819222 -
Flags: review?(dao)
|
|
Assignee | |
Comment 12•11 years ago
|
||
Whiteboard: [Australis:M?][Australis:P2] → [Australis:M9][Australis:P2][fixed-in-ux]
|
|
||
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/646a768dd809
https://hg.mozilla.org/mozilla-central/rev/9c0f89578118
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:M9][Australis:P2][fixed-in-ux] → [Australis:M9][Australis:P2]
Target Milestone: --- → Firefox 28
|
||
Comment 14•11 years ago
|
||
this patch need to check in the new _adjustElementStartAndEnd method that the scrollbox orient is horizontal
with the current patch if the orient is vertical you end up offsetting tab's top/bottom with left/right margin
You need to log in
before you can comment on or make changes to this bug.
Description
•