Closed Bug 1328030 Opened 3 years ago Closed 3 years ago

Drag selection(word by word selection) is broken if input is there

Categories

(Core :: Selection, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- fixed

People

(Reporter: arni2033, Assigned: mats)

References

(Blocks 1 open bug)

Details

(Keywords: regression, testcase)

Attachments

(6 files)

>>>
+++ This bug was initially created as a clone of Bug #666618 +++

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160517030211
STR_1:
1. Copy string "asdf" to clipboard. Open attached "testcase 1"
2. Move mouse over word "Stop", doubleclick without releasing left mouse button to start selection
3. Move mouse between "the" and "thing", then move mouse vertically through input to the next line
4.(bonus) Release left mouse button. Press Ctrl+C, select "sly" in input, press Ctrl+V


STR_2:
1. Copy string "asdf" to clipboard. Open attached "testcase 1"
2. Move mouse over word "Another", doubleclick without releasing left mouse button to start selection
3. Move mouse over the word "bug", then move mouse vertically to the line with "67890"
4.(bonus) Release left mouse button. Press Ctrl+C, select "efg" in input, press Ctrl+V


AR:
 In step 3 selection stops updating. In Step 4 text in <input> is highlighted with blue simultaneously
 with text in the line above; wrong test is pasted in <input>.
 
ER:  
 In Step 3 selection should update to contain second line. In Step 4 (only!) highlighted text should
 be copied to clipboard. Text in <input> shouldn't be highlighted simultaneously with text on page.


This is regression. The behavior has changed 3 times - (1), (2), (3)
1) (bug 372086) everything worked perfectly -> LEFT side of input became affected
2)              LEFT side of input was affected -> both sides of input became affected
3) (bug 762764) both sides of input were affected -> RIGHT side of input became affected

> [1] (between 2007-02-28 and 2007-03-01)   bug list: https://bugzilla.mozilla.org/buglist.cgi?list_id=13031457&resolution=---&resolution=FIXED&resolution=INVALID&resolution=WONTFIX&resolution=DUPLICATE&resolution=WORKSFORME&resolution=INCOMPLETE&resolution=SUPPORT&resolution=EXPIRED&resolution=MOVED&chfieldto=2007-03-01&query_format=advanced&chfield=resolution&chfieldfrom=2007-02-28&chfieldvalue=FIXED&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED
> [2] (between 2010-11-04 and 2010-11-05)   pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f7016571b472&tochange=5947e95a21d1
> [3] (between 2012-06-20 and 2012-06-21)   pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c3190d715044&tochange=10e019421e6b
No longer blocks: 1277113
Component: Untriaged → DOM: Core & HTML
Product: Firefox → Core
Ehsan, do you have time to take a look?
Flags: needinfo?(ehsan)
Is this a dup of a bug mats was fixing?
Or a variant of Bug 1327902.
I'll take a look...
Assignee: nobody → mats
Flags: needinfo?(ehsan)
Attached file debugging for STR_1
Here's what happens for STR_1:
At some point we have a selected range spanning from the start point
("Stop ..." / offset 0) to the end point (<td> / offset 1).
The <td>'s only child is an <input>.
When we drag below this end point, we get a call to nsIFrame::PeekOffset
with eDirNext, which calls GetFrameFromDirection -- this is what is being
debugged in this log.

We setup a NS_NewFrameTraversal(... eLeaf ...) and call Next(),
which gives us the text node inside the anon <div> of the <input>,
which is selectable, so we return that as the result.

Then we fail similar to bug 1327902 - we try to Extend the selection to
that new end point, but it has a different "selection root" so we
clamp the range to that point.

Note, there is only ever one range involved in this case.
The general problem here is that we setup a range with one point
being inside an anonymous content tree, with the other point outside.
Attached patch fixSplinter Review
Attachment #8825046 - Flags: review?(bugs)
Attached patch testsSplinter Review
Component: DOM: Core & HTML → Selection
OS: Unspecified → All
Hardware: Unspecified → All
Blocks: 1329658
This bug only fixes word- and character-amount selection (mouse or keyboard).
I filed bug 1329658 for line-amount selection, which is different code that
probably needs a similar fix.
Comment on attachment 8825046 [details] [diff] [review]
fix

># HG changeset patch
># User Mats Palmgren <mats@mozilla.com>
># Parent  2622dac3b85ef205491bb2c9a6ca493dd860d7e3
>Bug 1328030 - Improve selecting content that contains elements with anonymous content (form controls etc).  r=smaug
>
>Two changes:
>In nsIFrame::GetFrameFromDirection, detect frames that are in
>a different anonynous
anonymous

 content tree than 'this' and then just keep
>traversing frames until we get one that isn't.
>
>In nsFrame::GetLastLeaf: the old code did allow the first child frame
>to be IsRootOfNativeAnonymousSubtree (it just checked siblings).
>I think this was unintentional and that we should check the first
>child too (and return its parent in that case, i.e. never return
>something that is IsRootOfNativeAnonymousSubtree here).
>
>diff --git a/layout/generic/nsFrame.cpp b/layout/generic/nsFrame.cpp
>--- a/layout/generic/nsFrame.cpp
>+++ b/layout/generic/nsFrame.cpp
>@@ -8310,22 +8310,37 @@ nsIFrame::GetFrameFromDirection(nsDirect
>                                   aVisual && presContext->BidiEnabled(),
>                                   aScrollViewStop,
>                                   true,  // aFollowOOFs
>                                   false  // aSkipPopupChecks
>                                   );
>     if (NS_FAILED(result))
>       return result;
> 
>-    if (aDirection == eDirNext)
>-      frameTraversal->Next();
>-    else
>-      frameTraversal->Prev();
>-
>-    traversedFrame = frameTraversal->CurrentItem();
>+    auto Advance = [&frameTraversal, aDirection] () {
>+      if (aDirection == eDirNext) {
>+        frameTraversal->Next();
>+      } else {
>+        frameTraversal->Prev();
>+      }
>+      return frameTraversal->CurrentItem();
>+    };
>+
>+    traversedFrame = Advance();
>+
>+    if (nsIContent* content = GetContent()) {
>+      // Advance until the frame is inside the same tree as 'this'.
>+      for (nsIContent* traversedContent;
>+           traversedFrame &&
>+             !traversedFrame->IsGeneratedContentFrame() &&
>+             (traversedContent = traversedFrame->GetContent()) &&
>+             !nsContentUtils::IsInSameAnonymousTree(content, traversedContent);) {
>+        traversedFrame = Advance();
>+      }
>+    }
So what if you have generated content frame in different anonymous tree?
Shouldn't you in IsGeneratedContentFrame() case check whether its content's parent is in same anonymous tree.
Could !traversedFrame->IsGeneratedContentFrame()  be
(!traversedFrame->IsGeneratedContentFrame() || !nsContentUtils::IsInSameAnonymousTree(traversedFrame->GetContent()->GetParent(), content)) 

> nsFrame::GetLastLeaf(nsPresContext* aPresContext, nsIFrame **aFrame)

GetLastLeaf is a tad weird. It is unclear to me what it actually tries to find, but at least your patch makes it more consistent.
The last frame or the last frame before first NAC or something like that.
Attachment #8825046 - Flags: review?(bugs) → review+
> Could !traversedFrame->IsGeneratedContentFrame()  be
> (!traversedFrame->IsGeneratedContentFrame() ||
> !nsContentUtils::IsInSameAnonymousTree(traversedFrame->GetContent()->GetParent(), content)) 

I tried it and it failed multiple tests in layout/base/tests/test_reftests_with_caret.html,
bug989012-1.html, bug989012-2.html at least.  Looks like stepping over an <img> that was
replaced by its alt-text failed with that change.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=919c78b93305711410c93d5bab2304d4deac8bdc

> The last frame or the last frame before first NAC or something like that.

Yes, the last frame where each of the ancestors is either the last sibling,
or is follow by a sibling that has content->IsRootOfNativeAnonymousSubtree().
This patch makes it more consistent by also stopping before the first child
if it has content->IsRootOfNativeAnonymousSubtree().

What's puzzling to me though is that nsFrame::GetFirstLeaf has none of these
checks.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e0c4890bcfb4
Improve selecting content that contains elements with anonymous content (form controls etc).  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9532ebc3faf0
Add basic tests that select content including form controls, using various mouse/kbd gestures.
Backed out for for breaking testAccessibleCarets.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6e044542d025e1e9f752ed40aa320f489942bf0
https://hg.mozilla.org/integration/mozilla-inbound/rev/e93051c16952c7726b2ad99bac8b6f267f985ae1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9532ebc3faf001a04f12b97cb9e95224898f22ff
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=67740926&repo=mozilla-inbound

[task 2017-01-10T21:15:53.531238Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets.js | [testAccessibleCarets : 227] [ 30 === 30 ] Selected phone number length should match expected maximum.
[task 2017-01-10T21:15:53.531424Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.531680Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | testAccessibleCarets.js - [testAccessibleCarets : 227] [ 30 === 30 ] Selected phone number length should match expected maximum.
[task 2017-01-10T21:15:53.531875Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=
[task 2017-01-10T21:15:53.532170Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets.js | [testAccessibleCarets : 231] [ null === null ] Focused element should match expected.
[task 2017-01-10T21:15:53.532384Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.532609Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | testAccessibleCarets.js - [testAccessibleCarets : 231] [ null === null ] Focused element should match expected.
[task 2017-01-10T21:15:53.532834Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=
[task 2017-01-10T21:15:53.533003Z] 21:15:53  WARNING -  TEST-UNEXPECTED-FAIL | testAccessibleCarets.js | [
[task 2017-01-10T21:15:53.533339Z] 21:15:53     INFO -  3 45 678 90 === 3 45 678 90 ] Selected phone number should match expected text. - See following stack:
[task 2017-01-10T21:15:53.533534Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.533770Z] 21:15:53     INFO -  Buffered messages finished
[task 2017-01-10T21:15:53.533987Z] 21:15:53  WARNING -  TEST-UNEXPECTED-FAIL | testAccessibleCarets | testAccessibleCarets.js - [
[task 2017-01-10T21:15:53.534285Z] 21:15:53     INFO -  3 45 678 90 === 3 45 678 90 ] Selected phone number should match expected text. - See following stack:
[task 2017-01-10T21:15:53.534549Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=JS frame :: robocop_head.js :: do_throw :: line 237
[task 2017-01-10T21:15:53.534706Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.534894Z] 21:15:53     INFO -  testAccessibleCarets.js | JS frame :: robocop_head.js :: do_throw :: line 237
[task 2017-01-10T21:15:53.535188Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=JS frame :: robocop_head.js :: do_report_result :: line 333
[task 2017-01-10T21:15:53.535410Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.535640Z] 21:15:53     INFO -  testAccessibleCarets.js | JS frame :: robocop_head.js :: do_report_result :: line 333
[task 2017-01-10T21:15:53.535887Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=JS frame :: robocop_head.js :: is :: line 349
[task 2017-01-10T21:15:53.536055Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.536281Z] 21:15:53     INFO -  testAccessibleCarets.js | JS frame :: robocop_head.js :: is :: line 349
[task 2017-01-10T21:15:53.536563Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=JS frame :: testAccessibleCarets.js :: testAccessibleCarets :: line 232
[task 2017-01-10T21:15:53.536717Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.536974Z] 21:15:53     INFO -  testAccessibleCarets.js | JS frame :: testAccessibleCarets.js :: testAccessibleCarets :: line 232
[task 2017-01-10T21:15:53.537254Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 319
[task 2017-01-10T21:15:53.537398Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.537634Z] 21:15:53     INFO -  testAccessibleCarets.js | JS frame :: resource://gre/modules/Task.jsm :: TaskImpl_run :: line 319
[task 2017-01-10T21:15:53.537867Z] 21:15:53     INFO -  TEST-PASS | testAccessibleCarets | Given message occurred for registered event: {innerType=progress, type=Robocop:Java, message=
[task 2017-01-10T21:15:53.538042Z] 21:15:53     INFO -  TEST-INFO | (xpcshell/head.js) | exiting test
[task 2017-01-10T21:15:53.538263Z] 21:15:53     INFO -  } - Robocop:Java should equal Robocop:Java
[task 2017-01-10T21:15:53.538472Z] 21:15:53     INFO -  (xpcshell/head.js) | exiting test
[task 2017-01-10T21:15:53.538663Z] 21:15:53     INFO -  EventExpecter: no longer listening for Robocop:Java
[task 2017-01-10T21:15:53.538930Z] 21:15:53     INFO -  Unregistered listener for Robocop:Java
[task 2017-01-10T21:15:53.539211Z] 21:15:53     INFO -  TEST-OK | testAccessibleCarets | took 41781ms
Flags: needinfo?(mats)
So, I'm guessing this broke the layout.accessiblecaret.extend_selection_for_phone_number
feature in some way.  Accessiblecaret selection in general seems fine though when
I test it on desktop (Linux) with:
layout.accessiblecaret.extend_selection_for_phone_number=true
layout.accessiblecaret.enabled=true
layout.accessiblecaret.hide_carets_for_mouse_input=false

I'm not sure I'm using the same gesture as the test though.  Is there a way
to generate a "long press" gesture in a desktop build?
or trigger the "extend_selection_for_phone_number" feature in some other way?

The test is calling getLongPressResult() here:
http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/mobile/android/tests/browser/robocop/testAccessibleCarets.js#110
which does touchstart+mouselongtap+touchend

The content this test is using:
mobile/android/tests/browser/robocop/testAccessibleCarets.html
Flags: needinfo?(mats) → needinfo?(tlin)
Looking at the raw log, it appears the only failure is:
\n\n\n\n3 45 678 90 === 3 45 678 90 ] Selected phone number should match expected text
which I guess is this one:
http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/mobile/android/tests/browser/robocop/testAccessibleCarets.js#232
and I guess it's clicking somewhere in " 678" in this element:
http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/mobile/android/tests/browser/robocop/testAccessibleCarets.html#42
(this appears to be a test for bug 1265750 BTW)
So I'm guessing the "extend_selection_for_phone_number" thing is now extending
the selection past the <input> on the left side, and that the \n's in the test
result is from the <br>'s before it...
Attached file phone number testcase
Here's a testcase that somewhat emulates what AccessibleCaretManager::ExtendPhoneNumberSelection is doing (result in console):

http://searchfox.org/mozilla-central/rev/225ab0637ed51b8b3f9f4ee2f9c339a37a65b626/layout/base/AccessibleCaretManager.cpp#933-970
I can see that there is a change in behavior with the patch.
Flags: needinfo?(tlin)
Here's a better testcase for the phone number match.
Without the fix in this bug, the selection is reset when reaching
the form control, and the phone number matching loop apparently
takes that as a sign to stop matching and return the last match.
The new behavior is that the space on the other side of the <input>
is selected, and spaces are matching the pattern so they are included.
That explains the three \n at the start of the test result.

Resetting the selection when reaching the <input> is exactly the kind
of bug that I'm intending to fix here and the new behavior is expected. 
The testAccessibleCarets.js test unfortunately depends on that bug.
(maybe it's intending to check is that the "12" inside the <input>
isn't selected as part of the phone number?)
(In reply to Mats Palmgren (:mats) from comment #15)
> So, I'm guessing this broke the
> layout.accessiblecaret.extend_selection_for_phone_number
> feature in some way.  Accessiblecaret selection in general seems fine though
> when
> I test it on desktop (Linux) with:
> layout.accessiblecaret.extend_selection_for_phone_number=true
> layout.accessiblecaret.enabled=true
> layout.accessiblecaret.hide_carets_for_mouse_input=false
> 
> I'm not sure I'm using the same gesture as the test though.  Is there a way
> to generate a "long press" gesture in a desktop build?
> or trigger the "extend_selection_for_phone_number" feature in some other way?

Yes. Turn on "layout.accessiblecaret.use_long_tap_injector" will inject a fake long tap event on desktop without touch event support. (All the other accessiblecaret prefs you already switched are needed as well.)
Yes, that seems to work, thanks!
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2982f9a486
Improve selecting content that contains elements with anonymous content (form controls etc).  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/1befb337dbd7
Add basic tests that select content including form controls, using various mouse/kbd gestures.
I've relanded with a small tweak to the testAccessibleCarets.html test to account
for the new behavior.  I just added an "x" before the <input> to stop the matching
there.  As I said, I think the intent of the test is to ensure the "12" inside
the <input> isn't included in the phone number.
Flags: in-testsuite+
Depends on: 1330526
https://hg.mozilla.org/mozilla-central/rev/5c2982f9a486
https://hg.mozilla.org/mozilla-central/rev/1befb337dbd7
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Worth taking this in 52? If so, please nominate for Aurora uplift :)
Flags: needinfo?(mats)
I don't think so - too risky change for little benefit.
Flags: needinfo?(mats)
Depends on: 1359411
Unfortunately, I had to back this out in bug 1359411 for now since it caused a hang
in TinyMCE in some cases.  I've filed a new bug 1362873 to re-fix this bug.
You need to log in before you can comment on or make changes to this bug.