Bug 148021 – Web Inspector: Style changes to Visual sidebar editors

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Modify the styles of the existing editors to fix bugs and positioning issues.

[Radar WebKit Bug Importer]

<rdar://problem/22283851>

[Devin Rousso]

Created attachment 258994 [details]
Patch

[Blaze Burg]

Comment on attachment 258994 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:35
> +        Due to a current bug, you cannot extend ES6 getters/setters.  In order to work around this, a member

Please link the bug. I have not heard about this, and we use it throughout the codebase.

If this is a JSC bug, you should annotate relevant code with the bug URL, so at a later time we will know why it was done that way (and if the workaround can be removed).

> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordCheckbox.css:47
> +    content: "Small Caps";

If we do this, then the string can't be localized.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleRelativeNumberSlider.js:70
> +            this._origin = this._startingValue;

Isn't a number line's origin always 0, in the mathematical sense?

This code seems to assign this._origin to outside the range represented by this._scale. I am confused by what this code is doing.

[Timothy Hatcher]

Comment on attachment 258994 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/VisualStyleKeywordCheckbox.css:47
>> +    content: "Small Caps";
> 
> If we do this, then the string can't be localized.

That is true, but we don't localize things that match the CSS keywords. We might change that stance after feedback, but I think "Small Caps" rendered in small caps makes sense here. The same effect wouldn't work if "Small Caps" was in Chinese or Japanese.

[Devin Rousso]

> > Source/WebInspectorUI/ChangeLog:35
> > +        Due to a current bug, you cannot extend ES6 getters/setters.  In order to work around this, a member
> 
> Please link the bug. I have not heard about this, and we use it throughout
> the codebase.
> 
> If this is a JSC bug, you should annotate relevant code with the bug URL, so
> at a later time we will know why it was done that way (and if the workaround
> can be removed).

https://bugs.webkit.org/show_bug.cgi?id=147064


> > Source/WebInspectorUI/UserInterface/Views/VisualStyleRelativeNumberSlider.js:70
> > +            this._origin = this._startingValue;
> 
> Isn't a number line's origin always 0, in the mathematical sense?
> 
> This code seems to assign this._origin to outside the range represented by
> this._scale. I am confused by what this code is doing.

The origin represents the offset of the value that isn't contained in the range.  For example, if the original number is 1000, we would want the origin to be 1000 so that when the user moves the slider in either direction they are adding to 1000.  Another example would be if the value is 50 and cannot go negative.  In this case, the origin would be 0 as the starting value is not outside of the slider's range.  I'll rename some of the variables to better explain this.

[Devin Rousso]

Created attachment 259015 [details]
Patch

[Blaze Burg]

Comment on attachment 259015 [details]
Patch

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

r=me, looking nice. I will add 147064 as a "blocker" so we get pinged when it's fixed.

> Source/WebInspectorUI/UserInterface/Views/VisualStyleRelativeNumberSlider.js:65
> +        let midpoint = this._scale / 2;

I guess the reason I found this code this confusing is that I would expect scale to be [min, max] not a scalar. It's fine for now, I guess.

[WebKit Commit Bot]

Comment on attachment 259015 [details]
Patch

Clearing flags on attachment: 259015

Committed r188490: <http://trac.webkit.org/changeset/188490>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.