Bug 168446 – Web Inspector: RTL: add TabBar support for RTL layout

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

It's a bit messy.

[Radar WebKit Bug Importer]

<rdar://problem/30559805>

[Blaze Burg]

Created attachment 301787 [details]
Patch

[Blaze Burg]

Created attachment 301789 [details]
After - RTL

To Test:
- Select different tabs, observe correct border and background color changes
- Observe correct static styles when window is inactive
- Drag a tab over other tabs, ensure correct styles and tab moves
- Close a tab using [X] button, tab bar shouldn't animate until mouseout of tab bar
- Close a tab should animate other tabs cleanly
- Adding a new tab with [+] button should animate cleanly from the left

[Devin Rousso]

Comment on attachment 301787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301787&action=review

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:109
> +body[dir=ltr] .tab-bar > :nth-child(n + 2 of .item),

NIT: should we put quotes around the attribute value?  I think that's a bit clearer.

body[dir="ltr"] .tab-bar > :nth-child(n + 2 of .item),

[Blaze Burg]

(In reply to comment #4)
> Comment on attachment 301787 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301787&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/TabBar.css:109
> > +body[dir=ltr] .tab-bar > :nth-child(n + 2 of .item),
> 
> NIT: should we put quotes around the attribute value?  I think that's a bit
> clearer.
> 
> body[dir="ltr"] .tab-bar > :nth-child(n + 2 of .item),

I don't think it actually matters. You are only ever going to see dir=ltr or dir=rtl on the body selector, so you quickly figure out that it's another spammy rule for RTL support =)

[Matt Baker]

Comment on attachment 301787 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=301787&action=review

r- for the layout issue, looks good otherwise. I really like the use of local CSS variables, rather than polluting the global namespace.

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:53
> +    --tab-item-light-border-style: 1px solid var(--tab-item-medium-border-color);

Shouldn't this be `var(--tab-item-light-border-color)`?

> Source/WebInspectorUI/UserInterface/Views/TabBar.css:310
> +    --tab-item-collapsed-close-button-margin-end: 0;

This variable isn't used anywhere. It should be `--tab-item-close-button-margin-end`, so that it overrides that value for collapsed tabs. Otherwise, hovering the tab causes the close button to appear and bump the layout over 4px.

> Source/WebInspectorUI/UserInterface/Views/TabBar.js:416
> +        return WebInspector.resolvedLayoutDirection() === WebInspector.LayoutDirection.LTR ? this._tabBarItems : this._tabBarItems.slice().reverse();

Nice.

[Blaze Burg]

Created attachment 301988 [details]
Patch v2

[WebKit Commit Bot]

Comment on attachment 301988 [details]
Patch v2

Clearing flags on attachment: 301988

Committed r212582: <http://trac.webkit.org/changeset/212582>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.