Bug 168446 - Web Inspector: RTL: add TabBar support for RTL layout
Summary: Web Inspector: RTL: add TabBar support for RTL layout
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: BJ Burg
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-02-16 11:00 PST by BJ Burg
Modified: 2017-02-17 14:56 PST (History)
4 users (show)

See Also:


Attachments
Patch (16.52 KB, patch)
2017-02-16 11:28 PST, BJ Burg
no flags Details | Formatted Diff | Diff
After - RTL (4.61 MB, video/quicktime)
2017-02-16 11:33 PST, BJ Burg
no flags Details
Patch v2 (17.67 KB, patch)
2017-02-17 14:25 PST, BJ Burg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2017-02-16 11:00:15 PST
It's a bit messy.
Comment 1 Radar WebKit Bug Importer 2017-02-16 11:01:08 PST
<rdar://problem/30559805>
Comment 2 BJ Burg 2017-02-16 11:28:04 PST
Created attachment 301787 [details]
Patch
Comment 3 BJ Burg 2017-02-16 11:33:44 PST
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
Comment 4 Devin Rousso 2017-02-16 11:55:46 PST
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),
Comment 5 BJ Burg 2017-02-16 21:28:46 PST
(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 =)
Comment 6 Matt Baker 2017-02-17 00:44:53 PST
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.
Comment 7 BJ Burg 2017-02-17 14:25:52 PST
Created attachment 301988 [details]
Patch v2
Comment 8 WebKit Commit Bot 2017-02-17 14:56:36 PST
Comment on attachment 301988 [details]
Patch v2

Clearing flags on attachment: 301988

Committed r212582: <http://trac.webkit.org/changeset/212582>
Comment 9 WebKit Commit Bot 2017-02-17 14:56:42 PST
All reviewed patches have been landed.  Closing bug.