Bug 168744 - Web Inspector: RTL: DOM Tree spacing is wrong
Summary: Web Inspector: RTL: DOM Tree spacing is wrong
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: Devin Rousso
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-22 14:36 PST by Devin Rousso
Modified: 2017-02-23 12:26 PST (History)
3 users (show)

See Also:


Attachments
[Image] Screenshot of Issue (164.25 KB, image/png)
2017-02-22 14:36 PST, Devin Rousso
no flags Details
Patch (10.43 KB, patch)
2017-02-22 15:39 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (396.93 KB, image/png)
2017-02-23 09:52 PST, Devin Rousso
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2017-02-22 14:36:31 PST
Created attachment 302443 [details]
[Image] Screenshot of Issue

.
Comment 1 Devin Rousso 2017-02-22 15:39:58 PST
Created attachment 302452 [details]
Patch
Comment 2 BJ Burg 2017-02-22 16:24:14 PST
Please include screenshots of the UI in RTL and LTR with patch applied.
Comment 3 Devin Rousso 2017-02-23 09:52:49 PST
Created attachment 302523 [details]
[Image] After Patch is applied
Comment 4 BJ Burg 2017-02-23 11:13:21 PST
Comment on attachment 302452 [details]
Patch

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

r=me. Impressive work!

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:105
> +    padding-right: calc(var(--sublist-margin-start) + var(--sublist-padding-start) + var(--sublist-border-width-start));

Very nice.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:117
> +    border: 0 solid hsla(0, 0%, 83%, 0.5);

Do we have an existing color variable for this? Not sure what the AppKit equivalent is...

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:133
> +    border-color: hsl(0, 0%, 83%);

Ditto.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:145
> +    padding: 0 var(--item-padding-start) 0 var(--item-padding-end);

I prefer explicit padding-side properties because it's easier to match up left + start and right + end. But I guess this is ok.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:204
> +    margin-right: calc(-1 * var(--sublist-margin-start) - var(--sublist-border-width-start));

Clever.
Comment 5 Devin Rousso 2017-02-23 11:56:51 PST
Comment on attachment 302452 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:117
>> +    border: 0 solid hsla(0, 0%, 83%, 0.5);
> 
> Do we have an existing color variable for this? Not sure what the AppKit equivalent is...

We don't, and this is also the only place that it's used.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.css:133
>> +    border-color: hsl(0, 0%, 83%);
> 
> Ditto.

We do, but it doesn't really apply for this case: --selected-background-color-unfocused in Variables.css
Comment 6 WebKit Commit Bot 2017-02-23 12:25:58 PST
Comment on attachment 302452 [details]
Patch

Clearing flags on attachment: 302452

Committed r212913: <http://trac.webkit.org/changeset/212913>
Comment 7 WebKit Commit Bot 2017-02-23 12:26:02 PST
All reviewed patches have been landed.  Closing bug.