New CSS' Item bounds incorrectly include highlighting

VERIFIED FIXED

Status

defect
P2
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: mitcho, Assigned: mitcho)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [visual][good first bug][b8])

Attachments

(2 attachments, 7 obsolete attachments)

With the new CSS, TabItems and GroupItems' effective bounds for snapping seem to include that of their outer highlighting, which is incorrect. Either:

1. we change the CSS so that somehow the highlighting is outside of the tab div's bounds itself, or
2. we change the snapping code to compute and use the appropriate bounds instead.

I'm not entirely sure what the issue is prima facie, but I'll look into it.
Posted patch wip1 (obsolete) — Splinter Review
Well there's your problem right there (wip attached). 

Using the CSS margin to provide spacing info to our layout algorithm is a bad idea precisely for this reason; it actually affects our ability to lay things out properly.

I realize we want to help people theme Panorama, but margin isn't the solution in this case. I think it's best to get everything working properly for ff4.0 and tackle theme-ability for ff4.x.
Assignee: nobody → mitcho
By the way, Mitcho, I'm leaving this to you to sort out (unless you don't want it)... just didn't want you to think I'd taken the bug over because I made a patch.
Mitcho, do you agree with my assessment? Looks like it's causing issues in rtl-land as well (see bug 587248).
(In reply to comment #3)
> Mitcho, do you agree with my assessment? Looks like it's causing issues in
> rtl-land as well (see bug 587248).

Hmmmm... maybe it's not related to this after all. 

Anyway, curious what your thoughts are here.
Yeah, I do think some of the rtl layout stuff is affected by this, in particular UI.reset(). Please test that routine in both ltr and rtl when fixing this. To test in rtl, use the "force RTL" add-on.
Status: NEW → ASSIGNED
This still bugs me, and it's an easy fix!
Blocks: 627096
Posted image Looking good!
The solution was to remove the margins on .tab and instead add them back in .tabInGroupItem.
Attachment #478421 - Attachment is obsolete: true
Posted patch Trivial patch (obsolete) — Splinter Review
Pushing to try to get win and linux builds to test as well.
Attachment #506128 - Flags: review?(ian)
Posted patch Trivial patch v2 (obsolete) — Splinter Review
Last attachment included some debug code.

ps: does anyone know why the margins on tabs are twice as large on Mac than Windows + Linux? Maybe we can tighten it to 6px or 4px.
Attachment #506128 - Attachment is obsolete: true
Attachment #506130 - Flags: review?(ian)
Attachment #506128 - Flags: review?(ian)
Attachment #476703 - Attachment is obsolete: true
Failed mac try on bug 587503 test. Running just that one test in isolation locally makes it pass... most likely this reflects a general weakness of that test (bug 625273), due to it not running in its own independent window and thus being sensitive to the exact positioning of the singleton group from prior tests.

Changed the bug 587503 test here to run in a new window. Sent to try.
Attachment #506130 - Attachment is obsolete: true
Attachment #506188 - Flags: review?(ian)
Attachment #506130 - Flags: review?(ian)
Passed try.
Duplicate of this bug: 628159
Comment on attachment 506188 [details] [diff] [review]
Trivial patch v2, with update to test 587503

This patch doesn't include any test fix. 

I don't know why it's wider on the Mac... I'm fine with fixing that, or filing a new bug on it so the design folks can weigh in, or whatever you think is best. :)

r- for the test fix missing
Attachment #506188 - Flags: review?(ian) → review-
Attachment #506188 - Attachment is obsolete: true
Attachment #506589 - Flags: review?(ian)
Comment on attachment 506589 [details] [diff] [review]
Trivial patch v2, with update to test 587503, for real this time

>+  newWindowWithTabView(onTabViewWindowLoaded);
> }
> 
>-function onTabViewWindowLoaded() {
>-  window.removeEventListener("tabviewshown", onTabViewWindowLoaded, false);
>+function onTabViewWindowLoaded(win) {
>+  win.removeEventListener("tabviewshown", onTabViewWindowLoaded, false);

You don't need the removeEventListener anymore, right?

R+ with that fixed
Attachment #506589 - Flags: review?(ian) → review+
Posted patch Patch v2.1 (obsolete) — Splinter Review
Attachment #506589 - Attachment is obsolete: true
Attachment #507366 - Flags: approval2.0?
Did the latest version of this patch pass try server?
Comment on attachment 507366 [details] [diff] [review]
Patch v2.1

a=beltzner
Attachment #507366 - Flags: approval2.0? → approval2.0+
(In reply to comment #18)
> a=beltzner
I would have liked to have seen comment 17 answered before granting approval...
Attachment #507366 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to comment #17)
> Did the latest version of this patch pass try server?

Yes. http://tbpl.mozilla.org/?tree=MozillaTry&rev=ac2d97359e17
Patch doesn't currently apply.
Actually, trivially resolved, so pushed http://hg.mozilla.org/mozilla-central/rev/06d9df6ca0fa
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.