Bug 206783

Summary: Web Inspector: Color picker: when zoomed, crosshair is misplaced on clicking
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Bug Depends on: 206968    
Bug Blocks:    
Attachments:
Description Flags
[Video] Bug
none
Patch
hi: review-
[Video] With patch applied
none
Patch none

Description Nikita Vasilyev 2020-01-24 16:50:00 PST
Created attachment 388742 [details]
[Video] Bug

Steps:
1. Press Command-+ twice.
2. Click in the middle of the HSB color square of the color picker.

Actual:
The circle indicating the selected color is way on the left of the mouse cursor.

Expected:
The circle indicating the selected color is right under the mouse cursor.

Notes:
This is not a regression.
Comment 1 Nikita Vasilyev 2020-01-24 16:51:45 PST
```
let point = window.webkitConvertPointFromPageToNode(this._element, new WebKitPoint(event.pageX, event.pageY));
```

webkitConvertPointFromPageToNode incorrectly interpolates the values when zoomed.
Comment 2 Radar WebKit Bug Importer 2020-01-24 16:52:13 PST
<rdar://problem/58886599>
Comment 3 Nikita Vasilyev 2020-01-25 15:32:42 PST
Created attachment 388789 [details]
Patch
Comment 4 Nikita Vasilyev 2020-01-25 15:36:13 PST
Created attachment 388790 [details]
[Video] With patch applied
Comment 5 Devin Rousso 2020-01-28 17:42:34 PST
Comment on attachment 388789 [details]
Patch

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

r-, as this breaks the bezier editor in RTL

> Source/WebInspectorUI/UserInterface/Models/Geometry.js:44
> +        return new WI.Point(

Style: we don't do this kind of multi-line function calling, so please collapse it into a single line
```
    return new WI.Point(event.pageX - rect.x, event.pageY - rect.y);
```

> Source/WebInspectorUI/UserInterface/Views/ColorSquare.js:191
> +        let point = {

NIT: `point` isn't used anywhere else, so you could just inline it:
```
    this._setCrosshairPosition({
        x: event.pageX - rect.x,
        y: event.pageY - rect.y,
    });
```
Comment 6 Nikita Vasilyev 2020-01-28 18:08:57 PST
(In reply to Devin Rousso from comment #5)
> Comment on attachment 388789 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388789&action=review
> 
> r-, as this breaks the bezier editor in RTL

This happens because we mirror the content with CSS transforms.

body[dir=rtl] .bezier-editor > .bezier-container {
    transform: scaleX(-1);
}

I see we do this a lot but it's a bad practice from accessibility standpoint — it doesn't update the accessibility tree but reverses the order visually.

Currently, both accessibility and RTL support of WI are in a poor state, so I'll just fix one thing at a time.
Comment 7 Nikita Vasilyev 2020-01-28 18:24:10 PST
In RTL mode, input fields are in the reverse order from CSS.

In CSS: cubic-bezier(0, 0.5, 1, 0.5)
In the popover: [0.5] [1] [0.5] [0]

In RTL mode, people would have to context switch when editing values in CSS vs the popover. This is pretty confusing. In this particular case, I think we are only making interactions more confusing.
Comment 8 Nikita Vasilyev 2020-02-08 15:51:21 PST
Created attachment 390184 [details]
Patch

(In reply to Devin Rousso from comment #5)
> Comment on attachment 388789 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=388789&action=review
> 
> r-, as this breaks the bezier editor in RTL

Bezier editor is strictly LTR now (Bug 206968). Resetting to "r?".
Comment 9 WebKit Commit Bot 2020-02-14 14:45:12 PST
Comment on attachment 390184 [details]
Patch

Clearing flags on attachment: 390184

Committed r256647: <https://trac.webkit.org/changeset/256647>
Comment 10 WebKit Commit Bot 2020-02-14 14:45:14 PST
All reviewed patches have been landed.  Closing bug.