597931 - New CSS' Item bounds incorrectly include highlighting

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: A display of the incorrect trenching - groups seem to be a tad better, but not perfect

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.

Comment 1

[Ian Gilman [:iangilman]]

Attachment: wip1

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.

Comment 2

[Ian Gilman [:iangilman]]

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.

Comment 3

[Ian Gilman [:iangilman]]

Mitcho, do you agree with my assessment? Looks like it's causing issues in rtl-land as well (see bug 587248).

Comment 4

[Ian Gilman [:iangilman]]

(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.

Comment 5

[Ian Gilman [:iangilman]]

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.

Comment 6

[Ian Gilman [:iangilman]]

This still bugs me, and it's an easy fix!

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Looking good!

The solution was to remove the margins on .tab and instead add them back in .tabInGroupItem.

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Trivial patch

Pushing to try to get win and linux builds to test as well.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Trivial patch v2

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.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Trivial patch v2, with update to test 587503

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.

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Passed try.

Comment 13

[Ian Gilman [:iangilman]]

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

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Trivial patch v2, with update to test 587503, for real this time

Comment 15

[Ian Gilman [:iangilman]]

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

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v2.1

Comment 17

[Shawn Wilsher :sdwilsh]

Did the latest version of this patch pass try server?

Comment 18

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 507366 [details] [diff] [review]
Patch v2.1

a=beltzner

Comment 19

[Shawn Wilsher :sdwilsh]

(In reply to comment #18)
> a=beltzner
I would have liked to have seen comment 17 answered before granting approval...

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Comment 21

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #17)
> Did the latest version of this patch pass try server?

Yes. http://tbpl.mozilla.org/?tree=MozillaTry&rev=ac2d97359e17

Comment 22

[Jonathan Watt [:jwatt]]

Patch doesn't currently apply.

Comment 23

[Jonathan Watt [:jwatt]]

Actually, trivially resolved, so pushed http://hg.mozilla.org/mozilla-central/rev/06d9df6ca0fa

Comment 24

[Tracy Walker [:tracy]]

verified with nightly build of minefield.