Closed
Bug 860248
Opened 11 years ago
Closed 11 years ago
Defect - Tapping on content while selection is active clears the selection but leaves grippers behind
Categories
(Firefox for Metro Graveyard :: Input, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
Firefox 24
People
(Reporter: jimm, Assigned: jimm)
References
Details
(Whiteboard: [selection] feature=defect c=Content_features u=metro_firefox_user p=2)
Attachments
(1 file)
6.26 KB,
patch
|
ally
:
review+
|
Details | Diff | Splinter Review |
STR: 1) press-hold on some text in content (not in an input) to select it 2) expand out the markers a bit 3) tap once on the selected text result: selection is cleared but the monocles are left behind Not sure what the correct behavior is. In the past we've prevented clicks from clearing selection. But maybe this is more desired behavior. If we keep this we need to figure out a way to turn selection monocles off when the frame modifies selected content underneath us. What we might do is pull bug 851521 up and work on it. We could add a mode in the frame (via selection controller maybe?) that disables default click behavior. Then the front end could control what a tap on selection does. MarcoM, we need clarification here on what the behavior should be. From there we can figure out how best to solve this. Tentatively marking this as dependent on bug 851521.
Assignee | ||
Updated•11 years ago
|
Whiteboard: [selection]
Updated•11 years ago
|
Blocks: metrov1defect&change
Updated•11 years ago
|
Summary: Tapping on selected text in content clears selection → Defect - Tapping on selected text in content clears selection
Whiteboard: [selection] → [selection] feature=defect c=tbd u=tbd p=0
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jmathies
Whiteboard: [selection] feature=defect c=tbd u=tbd p=0 → [selection] feature=defect c=tbd u=tbd p=5
Comment 1•11 years ago
|
||
Jim's point estimate=5
Whiteboard: [selection] feature=defect c=tbd u=tbd p=5 → [selection] feature=defect c=tbd u=tbd p=0
Comment 2•11 years ago
|
||
Hi Asa, please see Jim's question in Comment #0 about clarification on what the correct behavior should be.
Flags: needinfo?(asa)
Comment 3•11 years ago
|
||
I'd like Yuan's input here. Also, what do IE and Chrome do?
Flags: needinfo?(asa)
Assignee | ||
Comment 4•11 years ago
|
||
(In reply to Asa Dotzler [:asa] from comment #3) > I'd like Yuan's input here. Also, what do IE and Chrome do? IE is a little different. A single tap on any selectable content will invoke selection. Then another single tap brings up the context menu. Clicking off any selection clears the selection. We rely on press-hold for context menus, which is akin to right-clicking on desktop. What we do when the users single or double taps on selection is open for discussion. Tapping off selection will clear like in IE.
Assignee | ||
Updated•11 years ago
|
Assignee: jmathies → nobody
Updated•11 years ago
|
Priority: -- → P2
Comment 5•11 years ago
|
||
Asa asked for yuan's input in comment 3, let's get that. :)
Flags: needinfo?(ywang)
Comment 6•11 years ago
|
||
Just noting that this behavior occurs even if you tap outside the selection. 1) Double-tap to select a word 2) Tap anywhere in content 3) Note that the grippers have not gone away, but selection is cleared Once you get into this state, the grippers don't act right: - Double-tapping to select another word will not cause the grippers' positions to update, though it will cause the word to become selected - Attempting to move the grippers with touch input will cause them to jump to wherever you tapped - Any mouse or keyboard input will cause the grippers to disappear
Summary: Defect - Tapping on selected text in content clears selection → Defect - Tapping on content while selection is active clears the selection but leaves grippers behind
Updated•11 years ago
|
Updated•11 years ago
|
Whiteboard: [selection] feature=defect c=tbd u=tbd p=0 → [selection] feature=defect c=Content_features u=metro_firefox_user p=0
Assignee | ||
Updated•11 years ago
|
Assignee: nobody → jmathies
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(ywang)
Updated•11 years ago
|
Blocks: metrov1it9
Updated•11 years ago
|
No longer blocks: metrov1defect&change
Updated•11 years ago
|
QA Contact: jbecerra
Whiteboard: [selection] feature=defect c=Content_features u=metro_firefox_user p=0 → [selection] feature=defect c=Content_features u=metro_firefox_user p=5
Assignee | ||
Updated•11 years ago
|
Whiteboard: [selection] feature=defect c=Content_features u=metro_firefox_user p=5 → [selection] feature=defect c=Content_features u=metro_firefox_user p=2
Assignee | ||
Comment 7•11 years ago
|
||
Updated•11 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•11 years ago
|
||
Comment on attachment 761650 [details] [diff] [review] fix plus tests More touch up work on the click handler code when selection is active. This correctly clears content selection when you tap anywhere in content.
Attachment #761650 -
Flags: review?(ally)
Comment 9•11 years ago
|
||
Comment on attachment 761650 [details] [diff] [review] fix plus tests Review of attachment 761650 [details] [diff] [review]: ----------------------------------------------------------------- Looks mostly good. One nit & two caveats. The first caveat is the test, the second is that I don't see the feedback that Asa requests from Yuan on the bug itself. I assume since you sent it to review that you've worked it out offline. ::: browser/metro/base/content/helperui/SelectionHelperUI.js @@ +831,5 @@ > this.closeEditSession(false); > return; > } > > + if (this._hitTestSelection(aEvent) && this._targetIsEditable) { I'm pleased to see a patch removing more code than it is adding. :) @@ +850,4 @@ > } > > + // Close when we get a single tap in content. > + this.closeEditSession(false); nit: humor me & switch this to an if/else rather than an if block with an early return ::: browser/metro/base/tests/mochitest/browser_selection_basic.js @@ -311,2 @@ > gTests.push({ > - desc: "double-tap copy text in content", So, the test changes look sane, but I don't have your experience with the test framework. If you are confident in them, go ahead; if not you will need to seek a second reviewer.
Attachment #761650 -
Flags: review?(ally) → review+
Comment 10•11 years ago
|
||
We discussed the desired behavior in #windev, the behavior of the patch is what yuan has requested.
Assignee | ||
Comment 12•11 years ago
|
||
> @@ +850,4 @@
> > }
> >
> > + // Close when we get a single tap in content.
> > + this.closeEditSession(false);
>
> nit: humor me & switch this to an if/else rather than an if block with an
> early return
Hmmmm. :) I disagree with using this code pattern in our code, so I'm not sure I can. Although if this is common in our front end code I would switch to it. Will try to find the answer.
Assignee | ||
Comment 13•11 years ago
|
||
(In reply to Jim Mathies [:jimm] from comment #12) > > @@ +850,4 @@ > > > } > > > > > > + // Close when we get a single tap in content. > > > + this.closeEditSession(false); > > > > nit: humor me & switch this to an if/else rather than an if block with an > > early return > > Hmmmm. :) I disagree with using this code pattern in our code, so I'm not > sure I can. Although if this is common in our front end code I would switch > to it. Will try to find the answer. Our coding standard doesn't mention this explicitly, but it definitely leans toward early return in most cases - https://developer.mozilla.org/en-US/docs/Developer_Guide/Coding_Style
Assignee | ||
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fcf27567179
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0fcf27567179
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 24
Comment 16•11 years ago
|
||
Went through the following "Defect" for iteration #9 testing without any issues. Used the following build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-07-10-03-02-05-mozilla-central/ - Went through the original test case from comment 0 without any issues (grippers are being cleared) - Went through the problems reported in comment 6 without any issues (selected several words, selected more text after selecting words) - Selected some text and then created a tab using "+" at the top and the overlay on the right side, went back to the website and ensured the text was still selected (monocle grippers where not visible but this matches Firefox Desktop behavior) - Ensured that both monocle grippers disappeared when scrolling through the webpage - Ensured that mouse selections still worked in combination with touch selection without any issues (going between them) - Ensured that all of the above test cases worked in "Filled" view without issues
Comment 17•11 years ago
|
||
Went through the following "Defect" for iteration #10 testing without any issues. Used the following build: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2013-07-18-03-02-01-mozilla-central/ - Went through the original test case from comment 0 without any issues (grippers are being cleared) - Went through all the test cases added in comment 16 without any issues
Comment 18•11 years ago
|
||
User Agent: Mozilla/5.0 (Windows NT 6.2; Win64; x64; rv:26.0) Gecko/20100101 Firefox/26.0 Build ID: 20130806104538 Built from http://hg.mozilla.org/mozilla-central/rev/1e381c91885d Didn't WFM Tested on windows 8 using latest nightly for iteration-11. STR: 1) press-hold on some text in content (not in an input) to select it 2) expand out the markers a bit 3) tap once on the selected text result: selection is cleared but the monocles are left behind
Comment 19•11 years ago
|
||
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 Build ID: 20130816030205 Built from http://hg.mozilla.org/mozilla-central/rev/1ed5a88cd4d0 WFM Tested on windows 8 using latest nightly for iteration-12. Followed steps provided in comment0 and got expected result.
Status: RESOLVED → VERIFIED
Comment 20•11 years ago
|
||
Sometimes I see this issue, not on every site. See bug 904957.
Updated•10 years ago
|
OS: Windows 8 Metro → Windows 8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•