Bug 206783 - Web Inspector: Color picker: when zoomed, crosshair is misplaced on clicking
Summary: Web Inspector: Color picker: when zoomed, crosshair is misplaced on clicking
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: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on: 206968
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-24 16:50 PST by Nikita Vasilyev
Modified: 2020-02-14 14:45 PST (History)
5 users (show)

See Also:


Attachments
[Video] Bug (3.96 MB, video/quicktime)
2020-01-24 16:50 PST, Nikita Vasilyev
no flags Details
Patch (4.41 KB, patch)
2020-01-25 15:32 PST, Nikita Vasilyev
hi: review-
Details | Formatted Diff | Diff
[Video] With patch applied (652.64 KB, video/quicktime)
2020-01-25 15:36 PST, Nikita Vasilyev
no flags Details
Patch (4.44 KB, patch)
2020-02-08 15:51 PST, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.