Bug 35625 – Correct base/extent of selection on double-click.

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: New Bugs

[Ojan Vafai]

Correct base/extent of selection on double-click.

[Ojan Vafai]

Created attachment 49872 [details]
Patch

[Ojan Vafai]

Comment on attachment 49872 [details]
Patch

This patch depends on the patch in bug 35368.

[Alexey Proskuryakov]

Comment on attachment 49872 [details]
Patch

This patch has malformed ChangeLog.

+// FIXME: This method shouldn't be necessary. start/end should just be a function of base/extent
+// instead of separately stored values.

I don't understand. Why? These are completely different values, all visible through DOM APIs. See for example a test at <https://bug-4904-attachments.webkit.org/attachment.cgi?id=4806>.

r- for the formal reason of a broken ChangeLog, but I also have strong doubts about validity of the direction taken here, especially since it;s not explained at all.

[Ojan Vafai]

(In reply to comment #3)
> (From update of attachment 49872 [details])
> This patch has malformed ChangeLog.

Yeah, that's because it depends on the patch bug 35368.

> +// FIXME: This method shouldn't be necessary. start/end should just be a
> function of base/extent
> +// instead of separately stored values.
> 
> I don't understand. Why? These are completely different values, all visible
> through DOM APIs. See for example a test at
> <https://bug-4904-attachments.webkit.org/attachment.cgi?id=4806>.

I guess I don't really understand why it makes sense to expose these as different from anchor/focus. Why is is correct that, when you double-click, the base/extent do not actually correspond to the start/end of the selection?

[Alexey Proskuryakov]

Anchor/focus sound like they could be the same things as base/extent, but they actually are not. The former tell what the selection is, while the latter tell how it was created.

So, base is the DOM position where mouse pointer was when the user began creating the selection, and
extent is the DOM Position where the mouse was when the user finalized the selection.

It's expected behavior that base/extent are equal after a double-click. They will not be equal, but will still be different from anchor/focus after 1.5-clicking and dragging over text. I'm surprised we don't have tests verifying this - we should!

[Alexey Proskuryakov]

I don't have a great answer why it makes sense to expose these positions - but it's definitely way too late to change the API, and the more interesting anchor/offset ones are already exposed.

[Ojan Vafai]

> I don't have a great answer why it makes sense to expose these positions - but
> it's definitely way too late to change the API, and the more interesting
> anchor/offset ones are already exposed.

I'd really be surprised if anyone actually uses these correctly. I'd expect very few people to understand the difference between base/extent and focus/anchor, so more likely, if they are using them, it's causing them bugs.

> Anchor/focus sound like they could be the same things as base/extent, but they
> actually are not. The former tell what the selection is, while the latter tell
> how it was created.

Yup. Not sure why it's worthwhile exposing this. Seems like implementation details.

In either case, I'm more interested in us not using it internally than whether we expose it or not. The only time we need base/extent within WebCore instead of anchor/focus is in the EventHandler code that deals with mouse-based selections.

How about I rework this patch to do the following:
1) Leave base/extent exposed to the web as it is currently in trunk.
2) Store anchor/focus in VisibleSelection.
3) Use anchor/focus everywhere in our code except for functions the expose base/extent to the web.

At a quick glance at the selection code, I was able to find a bug with base/extent. I'll attach a testcase in a sec. I'd be surprised if we didn't have many bugs around base/extent not being at the bounds of the selection.

[Ojan Vafai]

Created attachment 51164 [details]
case that breaks due to base/extent

[Alexey Proskuryakov]

Sounds reasonable to me (might not be worth much, as I don't have the design of related classes internalized). Please make regression tests for each bugfix, and also for the current base/extent behavior.

[Nathan Vander Wilt]

This issue may be causing a discrepancy I'm seeing when dealing with user selections made by double-clicking (versus other input methods). When the DOM is modified with a broken selection as current, it gets collapsed: https://bugs.webkit.org/show_bug.cgi?id=114227

[Ahmad Saleem]

I am able to reproduce this bug in Safari 16.1, Safari Technology Preview 158 using attached test case and while doubling clicking from middle of word "middle", the selection only extent from middle rather than starting from the start of the word like other browsers Chrome Canary 110 and Firefox Nightly 109. Thanks!

[Ahmad Saleem]

It seems to be fixed now in Safari Technology Preview 165 and WebKit ToT and the selection start from ‘middle’ word and it might be something related to “Selection API” changes by Ryosuke.

Marking this as “RESOLVED CONFIGURATION CHANGED”.