1696176 - Broken behavior in nsIFrame::SelectByTypeAtPoint

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Kew [:jfkthame]]

The method nsIFrame::SelectByTypeAtPoint is supposed to select content at the given position, based on the nsSelectionAmount parameters to determine the types of boundary to respect in each direction.

The only reasonable expectation would be, for example, that if the given point falls in the middle of a simple glyph, and the requested amount is "character", the individual character at that position should be selected.

However, in this situation SelectByTypeAtPoint will in fact select two characters, the real "target" of the point and an additional character before or after it.

This behavior is reflected in the expectations of the nsIDOMWindowUtils.selectAtPoint testcase at https://searchfox.org/mozilla-central/rev/3f97afc8db535f9b0232222cb48cc4cbf8334c76/dom/tests/mochitest/chrome/selectAtPoint.html#113-119, where individual "character" or "cluster" selections apparently expect to select a pair of characters. I believe this expectation is incorrect/illogical.

A similar selecting-two-units-instead-of-one effect can also be demonstrated in Firefox on macOS (or presumably on other platforms if layout.word_select.eat_space_to_next_word is false; note that on Windows it is true by default).

STR:
In Firefox on macOS, load the trivial testcase

data:text/html,<h1>one two three

• Try double-clicking within a word. The individual target word is selected, as expected.

• Try double-clicking at the left-hand edge of the word "two". Again, the individual target word is selected, as expected.

• Now try double-clicking at the right-hand edge of "two". This time, Firefox selects "two three" -- it incorrectly extends the selection to include the following word. This happens if the double-click occurs within the right-hand half of the "o" or the left-hand half of the inter-word space.

It may be debatable whether double-clicking within inter-word space should in fact select the space instead of the adjacent word (this is what other browsers seem to do), but it certainly should not select both the preceding and following words.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1696176 - Fix nsIFrame::PeekBackwardAndForward so that selectAtPoint correctly selects a single character or cluster rather than two adjacent characters. r=dholbert

This also prevents incorrectly selecting two words when double-clicking at
the end of the first word (before the inter-word space).

We also update the selectAtPoint testcase to target more widely-spread glyphs,
to check that it is behaving accurately across a larger distance.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7ceef946175
Fix nsIFrame::PeekBackwardAndForward so that selectAtPoint correctly selects a single character or cluster rather than two adjacent characters. r=dholbert,emilio

Comment 3

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 3 changesets (bug 1696176, bug 1342741) for test_nsIHTMLEditor_getSelectedElement.html and inert-retargeting-iframe.tentative.html failures.

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&fromchange=a0f0682883e1db93f88a6ed4b87ee79bf2d22d31&test_paths=editor%2Flibeditor%2Ftests&selectedTaskRun=ZKyhNDRQQPiqjsuMQSi8UA.0&tochange=d1e50e1cfdd8f73c5f5d6516e96dd1963d159061
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&fromchange=2760de845c282752271101abe78c834d82304587&test_paths=inert&tochange=d1e50e1cfdd8f73c5f5d6516e96dd1963d159061&selectedTaskRun=HfZU9jWEQs6QheO_LtXahw.0
https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&fromchange=a0f0682883e1db93f88a6ed4b87ee79bf2d22d31&selectedTaskRun=B873jyEMRPqzyGw0FtQakg.0&test_paths=pointerevents%2Fcompat&tochange=d1e50e1cfdd8f73c5f5d6516e96dd1963d159061

Backout link: https://hg.mozilla.org/integration/autoland/rev/d1e50e1cfdd8f73c5f5d6516e96dd1963d159061

Failures logs:
https://treeherder.mozilla.org/logviewer?job_id=332707338&repo=autoland&lineNumber=8715

https://treeherder.mozilla.org/logviewer?job_id=332703877&repo=autoland&lineNumber=2496

https://treeherder.mozilla.org/logviewer?job_id=332699089&repo=autoland&lineNumber=4717

https://treeherder.mozilla.org/logviewer?job_id=332704117&repo=autoland&lineNumber=17178

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7e50a94db5b
Fix nsIFrame::PeekBackwardAndForward so that selectAtPoint correctly selects a single character or cluster rather than two adjacent characters. r=dholbert,emilio

Comment 5

[Cristian Brindusan [:cbrindusan]]

Backed out changeset d7e50a94db5b (bug 1696176) for bc failures in browser_edit.js.
https://hg.mozilla.org/integration/autoland/rev/4e5383da12377b323b67605c14b9bacc44a5ea9f

Push with failures:
https://treeherder.mozilla.org/jobs?repo=autoland&revision=d7e50a94db5b6dc772167bec411805aa7d937318&selectedTaskRun=fHbBADfuQ4--kI16buNzEA.0

Failure log:
https://treeherder.mozilla.org/logviewer?job_id=332790138&repo=autoland&lineNumber=7019

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad40c13d6a3d
Fix nsIFrame::PeekBackwardAndForward so that selectAtPoint correctly selects a single character or cluster rather than two adjacent characters. r=dholbert,emilio

Comment 7

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/ad40c13d6a3d