Closed Bug 528364 Opened 12 years ago Closed 12 years ago

Switching between tab previews inside the all tabs panel moves them slightly up/down by 1px

Categories

(Firefox :: Tabbed Browser, defect, P2)

defect

Tracking

()

VERIFIED FIXED
Firefox 3.7a1
Tracking Status
status1.9.2 --- beta5-fixed

People

(Reporter: whimboo, Assigned: karlt)

References

Details

(Keywords: regression, verified1.9.2)

Attachments

(2 files)

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b3pre) Gecko/20091110 Namoroka/3.6b3pre ID:20091110034400

Steps:
1. Open a couple of tabs
2. Enable browser.allTabs.previews 
3. Open the all tabs panel
4. Use the tab key to switch between the previews

You will notice while navigating through the list of shown tab previews those boxes will slightly move up and down.
I think this is a regression. Would be nice to know when this regressed.
Blocks: 494901
Component: Tabbed Browser → XUL
Product: Firefox → Core
QA Contact: tabbed.browser → xptoolkit.widgets
Version: 3.6 Branch → 1.9.2 Branch
Doesn't seem to happen on Linux with m-c 5caa19335d5e.
Perhaps the difference is in the theming of the text input.
Does the movement only happen when the text input receives and loses focus?
i.e. not when focus changes between previews?
Thanks for the regression range. I have tested again and it's Windows and OS X only.
(In reply to comment #3)
> Doesn't seem to happen on Linux with m-c 5caa19335d5e.
> Perhaps the difference is in the theming of the text input.

I'm pretty sure the difference that's relevant for this bug is in the styling of focused previews.

> Does the movement only happen when the text input receives and loses focus?
> i.e. not when focus changes between previews?

It happens when focus changes between previews.
Flags: blocking1.9.2?
Karl, can you take this?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
I'll  look into it.
Assignee: nobody → karlt
Attached file testcase
This was the modification that caused the change in behavior:
https://bugzilla.mozilla.org/attachment.cgi?id=400842&action=diff#a/layout/xul/base/src/nsSprocketLayout.cpp_sec8

It corrected the baseline for hboxes with align=baseline.

I'm not sure there is a "correct" baseline for hboxes with other alignments and multiple children.

For hboxes with only one child, as in this testcase, the baseline of the child seems an appropriate baseline for the hbox.

For hboxes with (client-)height != child-height and align=center, this is not what nsSprocketLayout::GetAscent() returned either before or after
http://hg.mozilla.org/mozilla-central/rev/f163afecb7c3

For border-top-width != 0, f163afecb7c3 made the hbox baseline that of the
child when client-height == child-height, or when client-height != child-height
and align= "baseline" or "start" or sometimes "stretch".

When client-height != child-height and align=center, f163afecb7c3 means that
sometimes results are more consistent than before (when only border-top-width is
changed) and sometimes less consistent (when both botter-top-width and
border-bottom-width are changed by equal amounts), but this is only discussing
whether one wrong is better than another.
Flags: blocking1.9.2+ → blocking1.9.2?
Keywords: testcase-wanted
Summary: Switching between tab previews inside the all tabs panel moves them slightly up/down by 1px → Switching between tab previews inside the all tabs panel moves them slightly up/down by 1px (incorrect baseline for hbox with client-height != child-height and align=center)
I think we should reconsider whether this blocks from a Core::XUL POV.
This doesn't seem to be a clear regression from a XUL POV.
I don't think we want to start making more changes to baseline calculations
in 1.9.2 at this stage.

If this is considered a blocker from a browser perspective (for a feature that
is only available through about:config?), then I think the best fix would be a
Tabbed Browser or theme change so that positioning of the previews does not
depend on their baselines.

If there is good reason for allTabs-container to be display:block, then a
simple fix with be

    .allTabs-preview {
      vertical-align: bottom;
    }

(Or "top" or "middle" if one is more appropriate.)

But wouldn't it make more sense for allTabs-container to be an hbox
(without display:block)?
(In reply to comment #11)
> If there is good reason for allTabs-container to be display:block, ...

> But wouldn't it make more sense for allTabs-container to be an hbox
> (without display:block)?

I guess display:block is for the line-breaking.  It looks like allTabs guesses how many columns and rows there will be and then hopes that the block frame will lay out the previews in the same way.
The previews have fixed-height so it doesn't really matter whether this is top or bottom or middle.  I used top because it has fewer characters.
Attachment #415074 - Flags: review?(dao)
Attachment #415074 - Flags: review?(dao) → review+
I files Bug 531989 for the Core::XUL issue.
Moving this to Tabbed Browser for the symptoms and fix/workaround here.
Component: XUL → Tabbed Browser
Flags: blocking1.9.2?
Product: Core → Firefox
QA Contact: xptoolkit.widgets → tabbed.browser
Target Milestone: --- → Firefox 3.7a1
Version: 1.9.2 Branch → Trunk
blocking1.9.2+ -> blocking-firefox3.6?
Flags: blocking-firefox3.6?
Summary: Switching between tab previews inside the all tabs panel moves them slightly up/down by 1px (incorrect baseline for hbox with client-height != child-height and align=center) → Switching between tab previews inside the all tabs panel moves them slightly up/down by 1px
Flags: blocking-firefox3.6? → blocking-firefox3.6+
Whiteboard: [can land 1.9.2]
http://hg.mozilla.org/mozilla-central/rev/b7f3c1c9fb57
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/52b8170d428b
Status: NEW → RESOLVED
Closed: 12 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [can land 1.9.2]
Verified fixed on trunk and 1.9.2 with builds on OS X and Windows like

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.3a1pre) Gecko/20091220 Minefield/3.7a1pre ID:20091220030635

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2b6pre) Gecko/20091217 Namoroka/3.6b6pre ID:20091217033725
Status: RESOLVED → VERIFIED
Keywords: verified1.9.2
You need to log in before you can comment on or make changes to this bug.