Closed Bug 610685 Opened 14 years ago Closed 13 years ago

App tabs disappear in RTL firefox

Categories

(Firefox :: Tabbed Browser, defect)

Product:
Firefox
Component:
Tabbed Browser
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 4.0b12
Tracking Flags:
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: smontagu, Assigned: smontagu)

References

Details

(Keywords: rtl, Whiteboard: [hardblocker][has patch / fix in bug 508816])

Attachments

(2 files)

workaround (should be avoided if possible)
8.23 KB, patch
Details | Diff | Splinter Review
html testcase
416 bytes, text/html
Details 
Steps to reproduce:

Use an RTL localization, or set intl.uidirection.en to "rtl" in about:config
Open enough tabs to fill the window width and overflow it.
Pin one or more tabs as App Tabs.

Expected results:
App tab icons appear on the right edge of the tab bar

Actual results:
Empty space appears on the right edge of the tab bar, but no icons. The app tabs are still accessible by keyboard shortcuts, but are invisible.
Is this a recent regression or didn't it work from the beginnings?
blocking2.0: --- → ?
Version: unspecified → Trunk
It's broken since the earliest builds that implement app tabs.
Mozilla/5.0 (Windows NT 6.1; rv:2.0b8pre) Gecko/20101110 Firefox/4.0b8pre

Able to reproduce. Empty space appears on the right edge of the tab bar. Able to access the app tab by keyboard shortcut (ctrl+1), but the app tab is not shown. Only the window is shown. Unable to move to the right edge with the help of scroll keys. If i click and hold the left scroll key, it moves in the opposite direction and comes back to the left most tab.
Invisibility is not a feature -> blocking+.
blocking2.0: ? → final+
I haven't figured out why yet, but for some reason when we start to overflow, the margin-start point moves (or something...). The negative margins on the tab work fine. Then when the scroll buttons are shown, positive margins are needed (and it looks like the start point might be off screen?)

Dao - you know all of this code a lot better than I, any ideas?

Markus - You changed code around here recently (bug 594002) so it's not you, but you might have an idea.
Sounds like a layout bug...
blocking2.0: final+ → betaN+
I've been debugging this, the X position for the app tab grows with each tab added. the position is being calculated using the full tabbar width rather than only the scroll width.
Boris, can you give some help or pointers here? I believe it's a layout bug but I'm not sure yet. See comment 7, and in case it's not clear enough here's an ASCII drawing with my current understanding of what happens.


consider that regular tabs are 100px, and due to the window size the tab bar can display 3 tabs + 2 app tabs. The app tabs are normal <tab> elements children of the tabbar scrollbox, and to display them outside we use position:fixed and negative margins.

-----------------------------------
LTR case:


        scrollbox -moz-margin-start: 50px
         |

[*][*] < [*-----x][*-----x][*-----x] >

 ^
apptab  position: fixed
        -moz-margin-start: -50px  -->  final x position: 0



-----------------------------------
RTL (same CSS rules):

                           
< [*-----x][*-----x][*-----x] > [*][*]

                                |
                                final x position: 300px


the first app tab X position should be the width of the scrollbox. However, it grows by 100px for each tab I add, so it seems that it's being offset by the size of the entire content inside the scrollbox, rather than just by the scrollbox width.
This reminds me of bug 508816, although the symptoms seem rather different... GetScrolledRectInternal is called in a few different spots, so I guess it could cause various quirks...
> we use position:fixed and negative margins.

Sounds like layout is doing the right thing, then.

Since you're not specifying offsets, the fixed-position box is supposed to be positioned exactly where it would be positioned if it were static position, but be taken out of flow and not move when things are scrolled.  So in particular, in the rtl case you have some setup more or less equivalent to this:

  <div style="overflow: scroll; width: 320px; margin-right: 50px">
    <span style="display: inline-block; width: 100px"></span>
    <span style="display: inline-block; width: 100px"></span>
    <span style="display: inline-block; width: 100px"></span>
    <span style="position: fixed; margin-right: -50px">where am I?</span>
  </div>

The correct rendering of that is to have the "where am I" block 300px from the left of the left edge of the div, since that's where the static position is.  

In other words, the only reason this works in an ltr context for you is because scrolling happens to the right, so the static position is in view to start with.

What you really seem to be after here is fixed positioning that lets you position relative to a particular element (the scrollbox's parent).  There are some proposals for that sort of thing, but no spec or implementation yet....

Can you assume things about how the scrollbox is placed in the viewport?  If not, you may have to compute its position/size and style the app tabs via script accordingly...
Makes sense...
This should work on Linux and Windows, haven't tested Mac yet.
Assignee: nobody → dao
Status: NEW → ASSIGNED
Works fine on Mac. Thank you!
(In reply to comment #10)
> > we use position:fixed and negative margins.
> 
> Sounds like layout is doing the right thing, then.
> 
> Since you're not specifying offsets, the fixed-position box is supposed to be
> positioned exactly where it would be positioned if it were static position, but
> be taken out of flow and not move when things are scrolled.  So in particular,
> in the rtl case you have some setup more or less equivalent to this:
> 
>   <div style="overflow: scroll; width: 320px; margin-right: 50px">
>     <span style="display: inline-block; width: 100px"></span>
>     <span style="display: inline-block; width: 100px"></span>
>     <span style="display: inline-block; width: 100px"></span>
>     <span style="position: fixed; margin-right: -50px">where am I?</span>
>   </div>
> 
> The correct rendering of that is to have the "where am I" block 300px from the
> left of the left edge of the div, since that's where the static position is.
>
> In other words, the only reason this works in an ltr context for you is because
> scrolling happens to the right, so the static position is in view to start
> with.

Thinking about this again, it seems that the rtl case is significantly different from your example. Nothing being scrolled should mean the first tab is aligned to the right. So I would expect rtl to work just like ltr. The app tabs' static position should be in the view to start with, but bug 508816 prevents this.
> Nothing being scrolled should mean the first tab is aligned to the right

Yes, but what's the underlying layout data structure look like?  Does it think it's scrolled at that point?

> but bug 508816 prevents this

Like I said, it thinks it's scrolled at that point.
(In reply to comment #14)
> > Nothing being scrolled should mean the first tab is aligned to the right
> 
> Yes, but what's the underlying layout data structure look like?  Does it think
> it's scrolled at that point?

It probably does think so, but why would this be correct behavior?
It's certainly "correct" behavior for a CSS scroll area, which defaults to being at top/left no matter what it's direction is ("correct" in the "websites will break if you don't do it that way" sense).  Whether it's correct behavior for whatever construct you're using here depends on who else is using it and why.
Attached file html testcase — 
(In reply to comment #16)
> It's certainly "correct" behavior for a CSS scroll area, which defaults to
> being at top/left no matter what it's direction is

I don't understand what this means really, given that scroll areas in rtl HTML default top/right. I'm attaching an HTML testcase that shows how I would expect this to work in XUL too.
Huh.  I didn't think we did that....

In any case, are there layout issues here past bug 508816?
I don't think so, based on what I know so far.
Depends on: 508816
Comment on attachment 501629 [details] [diff] [review]
workaround (should be avoided if possible)

Repositioning app tabs when we know the scrollbox moves around and assuming the scrollbox won't move in other situations is pretty fragile. Should only be considered as the last resort.
Attachment #501629 - Attachment description: patch → workaround (should be avoided if possible)
Assignee: dao → nobody
Status: ASSIGNED → NEW
Whiteboard: [hardblocker]
(In reply to comment #20)
> Repositioning app tabs when we know the scrollbox moves around and assuming the
> scrollbox won't move in other situations is pretty fragile. Should only be
> considered as the last resort.

What are the possible side effects? Are they worse than the current behavior?
It would mean that the app tabs position could be off in some situations for LTR as well as RTL.
I've been investigating this bug and I would like to ask if we can't group the pinned tabs into an anonymous node? A node that we could style. If anonymous nodes can't be used for such purposes, then can't we go for a normal node where we group them?

I ask this because if we can group the pinned tabs into a single element, inside our outside the xul:tabs element, we can have better flexibility on the styling, and I believe work around the bug / fix the issue.
(In reply to comment #23)
> I've been investigating this bug and I would like to ask if we can't group the
> pinned tabs into an anonymous node? A node that we could style. If anonymous
> nodes can't be used for such purposes, then can't we go for a normal node where
> we group them?
> 
> I ask this because if we can group the pinned tabs into a single element,
> inside our outside the xul:tabs element, we can have better flexibility on the
> styling, and I believe work around the bug / fix the issue.

When app tabs were implemented in bug 563730 Unfocused wrote a patch to group the app tabs into a single element outside of the xul:tabs element, but Dão had concerns about that, so Dão went with using position: fixed. Given the issues that we're now running into, would the former approach provide a solution?
We don't want to open that can of worms. We really should just fix bug 508816.
(In reply to comment #25)
> We don't want to open that can of worms. We really should just fix bug 508816.

This is why I asked. I know going that route (a single element that groups the pinned tabs) can open a new set of possibilities for things to go wrong.

Frank: I believe that grouping the pinned tabs can provide us with a solution, but as Dao points out... it's risky this late in the cycle.
It sounds like fixing bug 508816 is the less risky approach, and also fixes the root cause of the problem. I'm going to mark it blocking+.
Whiteboard: [hardblocker] → [hardblocker][fix in bug 508816]
Simon, assigning this to you since it's fixed by your other bug. Any objections?
Assignee: nobody → smontagu
Whiteboard: [hardblocker][fix in bug 508816] → [hardblocker][has patch / fix in bug 508816]
Fixed by bug 508816. http://hg.mozilla.org/mozilla-central/rev/18a59f6f099b includes a version of attachment 501933 [details], but maybe we could do with more. Are there existing tests of tab behaviour which I could make an RTL version of?
Status: NEW → RESOLVED
Closed: 13 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
(In reply to comment #29)
> Are there existing tests of tab behaviour which I could make an RTL version of?

Not for app tab rendering.
Target Milestone: --- → Firefox 4.0b12
What about stuff like the tab bar scrolling to the right place when opening new tabs, switching tabs, etc?
Mozilla/5.0 (X11; Linux i686; rv:2.0b12) Gecko/20100101 Firefox/4.0b12

Verified issue and it's no longer present in latest build.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: