Closed Bug 739396 Opened 14 years ago Closed 11 years ago

The CSS rule "-moz-user-select: none" no longer disables text selection when using "Ctrl+a".

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Version:
11 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla35
Tracking Flags:
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- wontfix
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- wontfix
firefox25 --- wontfix
firefox-esr10 --- unaffected
firefox-esr17 --- wontfix

People

(Reporter: browning_11, Assigned: MatsPalmgren_bugz)

References

(Depends on 1 open bug, Blocks 1 open bug, Regressed 4 open bugs,
URL
)

Details

(Keywords: regression)

Attachments

(3 files, 7 obsolete files)

part 0, Make nsDocumentViewer use mozilla::dom::Selection internally instead of nsISelection.
6.53 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
part 2, tests
8.78 KB, patch
Details | Diff | Splinter Review
part 1, Exclude non-selectable nodes from the Selection.
19.36 KB, patch
MatsPalmgren_bugz
: review+
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (compatible; MSIE 9.0; Windows NT 6.1; WOW64; Trident/5.0; SLCC2; .NET CLR 2.0.50727; .NET CLR 3.5.30729; .NET CLR 3.0.30729; Media Center PC 6.0; .NET4.0C; .NET4.0E; InfoPath.1; AskTbTRL2/5.9.1.14019) Steps to reproduce: I pressed "Ctrl+a" to select all rows in a "web 2.0" style grid. This grid is in an application that runs in a browser. I have a CSS rule to disable text selection "-moz-user-select: none" on all elements on the page. Actual results: All text on the page becomes selected. I have also used a simple test to confirm this bug. <style> .unselectable { -moz-user-select: none; } </style> <body> <p class="unselectable">The user is not able to select this text in Firefox, Chrome, Safari, and IE.</p> </body> Expected results: No text should have been selected due to the "-moz-user-select: none" css rule.
This bug has been introduced in FireFox 11.0. It is not present in previous versions.
Regression window(m-c), Works: http://hg.mozilla.org/mozilla-central/rev/c7101dec8deb Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111220 Firefox/11.0a1 ID:20111220083550 Fails: http://hg.mozilla.org/mozilla-central/rev/a8506ab2c654 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111220 Firefox/11.0a1 ID:20111220085450 Pushlog: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c7101dec8deb&tochange=a8506ab2c654 Regression window(m-i), Works: http://hg.mozilla.org/integration/mozilla-inbound/rev/feaccb6a4c35 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111219 Firefox/11.0a1 ID:20111219235256 Fails: http://hg.mozilla.org/integration/mozilla-inbound/rev/0aa9c3a5b7e1 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0a1) Gecko/20111219 Firefox/11.0a1 ID:20111220011653 Pushlog: http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=feaccb6a4c35&tochange=0aa9c3a5b7e1 Triggered by: Bug 619273
Blocks: 619273
URL: data:text/html;charset=utf-8<!DOCTYPE...
Component: Untriaged → Selection
Keywords: regression
Product: Firefox → Core
QA Contact: untriaged → selection
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → matspal
Blocks: 816298
status-firefox-esr10: --- → unaffected
status-firefox18: --- → affected
status-firefox19: --- → affected
status-firefox20: --- → affected
status-firefox21: --- → affected
status-firefox-esr17: --- → affected
OS: Windows 7 → All
Hardware: x86_64 → All
I do not understand why this would not be the intended behavior. That option is designed to keep people from accidentally selecting text when dragging on application elements. It is not intended to keep people from being able to copy text at all. I mean, it's already being abused on websites that are not applications--sites that are so worried about plagiarism that they break basic browser functions. Disabling Ctrl-A will just make that abuse worse. What's next, disabling view source and inspect element?
(In reply to comment #4) > I do not understand why this would not be the intended behavior. That option is > designed to keep people from accidentally selecting text when dragging on > application elements. It is not intended to keep people from being able to copy > text at all. > > I mean, it's already being abused on websites that are not applications--sites > that are so worried about plagiarism that they break basic browser functions. > Disabling Ctrl-A will just make that abuse worse. What's next, disabling view > source and inspect element? As the user agent, we don't know when to trust websites on this and when not to, but my impression is that most web developers have stopped bothering with these "protection" mechanisms (remember the days of "disable right click" tutorials floating around? :-)
I'm an app developer and this behavior is annoying. Please fix it.
I can confirm that this is still an bug in firefox 27.0.1 While there are many sites that could abuse this, it's not the browsers responsibility to police the web and remove features. There is a whole raft of things in javascript which can be abused but that doesn't mean we cripple the language by removing those features. I have a legitimate use this this and I'm sure it's not the only one. I am developing a pastebin, I want to enable selection of the paste but not of the header bar at the top, allowing them to easily copy and paste large pastes without having to attempt to select the text. Please fix this.
Replying to bug 1032090 comment 2: (In reply to Robert O'Callahan) > Other browsers don't support multi-range selection at > all so we probably would never be able to standardize it. I don't think it has been decided yet. Multi-range selection came up in some discussions on public-webapps@ recently in the context of "contentEditable=minimal" and also selection actions that spans shadow trees. For example in: http://lists.w3.org/Archives/Public/public-webapps/2014AprJun/0971.html I don't know if there was a conclusion, but you can find the recent discussions here: http://lists.w3.org/Archives/Public/public-webapps/2014AprJun/thread.html We need some way to represent selection in tables (columns), BIDI text, and 'user-select:none'. (I don't know much about shadow trees, but perhaps they too need to be treated as non-selectable islands in some cases). I haven't seen any proposal on how to implement these use cases without multi-range selections.
I notice in Webkit browsers, the selection and bounding behavior show correctly: data:text/html,<body style="-webkit-user-select: none;"><p>hi</p><div style="-webkit-user-select: text;">hello world</div><p>hi</p><div style="-webkit-user-select: text;">hello world2</div></body> Hi Mat, do you have any idea how Webkit achieve this? Maybe can we do the same here then it would be solved? Thanks!
Flags: needinfo?(mats)
Excuse me for the name typo in Comment 10 , Mats! :-)
Well, Copy+Paste in WebKit (and Blink) includes the 2nd "hi" that isn't selected. That doesn't seem right. Same for the simpler test: data:text/html,<body>hello<p style="-webkit-user-select: none;">hi</p>world</body>
Flags: needinfo?(mats)
Firefox Copy+Paste excludes 2nd "hi" in this case, which behaves correctly; so in Firefox it's more like a display issue if we could just fix?
Flags: needinfo?(mats)
Before we debate this bug too much, it would be good if someone could file a *separate* bug which describes the set of UI use cases that we need for FirefoxOS. I'm not at all sure that solving those will involve using -moz-user-select: none, so in the end this bug might not end up getting involved.
This is just an experiment to see how hard it would be to implement the multi-range solution. Simple selections gestures appears to work properly: drag-select from A to B over an -mos-user-select:none node, copy+paste the resulting selection; select-all + copy + paste; etc. It needs more work, but it seems like a viable solution to me. https://tbpl.mozilla.org/?tree=Try&rev=0c95d24038b1
Flags: needinfo?(mats)
(In reply to Jonas Sicking (:sicking) from comment #14) > Before we debate this bug too much, it would be good if someone could file a > *separate* bug which describes the set of UI use cases that we need for > FirefoxOS. I'm not at all sure that solving those will involve using > -moz-user-select: none, so in the end this bug might not end up getting > involved. I filed bug 1036853 to depict another use case that I think could be more useful.
Firefox OS UI use case draft is in bug 1037286, based on the use cases UI provided now, we might need to get this and/or bug 1036853 fixed.
I fixed an issue when shortening the selection by Shift+Clicking inside it. In the multi-range case we need to also remove ranges that starts beyond the click point. And fixed some repaint issues. howie/timdream feel free to try these two patches and see if it fixes your use cases with -moz-user-select:none.
Attachment #8453039 - Attachment is obsolete: true
howie, if you need Try builds for testing, you can find some at: https://tbpl.mozilla.org/?tree=Try&rev=13efb0ba2319
Hi Mats, Tim and I tried the patches and it looks good! This fix is definitely helpful for Firefox OS to take the next step, I guess we can proceed the review process? Since UX discussion is ongoing, we might still need your help afterwards(e.g. bug 1036853), but really appreciate your support so far! :-)
Thanks, that sounds good. I'll be away on vacation for several weeks now though. Maybe roc and/or smaug can help while I'm away?
Hi Robert, hi Olli, may I ask your help for this bug? Mats already had the patch and we tried it looks good. Since Mats will be away for weeks, is it possible that you could take over current patch and help to finish test case/review? Many thanks and your help means a lot to the copy/paste feature in Firefox OS!
Flags: needinfo?(roc)
Flags: needinfo?(bugs)
Comment on attachment 8456819 [details] [diff] [review] part 0, Make nsDocumentViewer use mozilla::dom::Selection internally instead of nsISelection. Adding to my review queue.
Attachment #8456819 - Flags: review?(bugs)
Attachment #8456828 - Flags: review?(bugs)
Flags: needinfo?(bugs)
howie, do you think you could find someone to write some testcases (in form of mochitest), perhaps based on the requirements from FFOS?
Flags: needinfo?(hochang)
Comment on attachment 8456819 [details] [diff] [review] part 0, Make nsDocumentViewer use mozilla::dom::Selection internally instead of nsISelection. >+ >+ // use nsCopySupport::etSelectionForCopy() ? s/et/Get/
Attachment #8456819 - Flags: review?(bugs) → review+
fyi- I've spotted an odd issue w/Mobile TextSelection https://bug1041221.bugzilla.mozilla.org/attachment.cgi?id=8459222 I don't know if it's this bug, or other core:TextSelection work I've been observing, or something else we simply did to ourselves yet, but please be aware -- mark
(In reply to Olli Pettay [:smaug] from comment #26) > howie, do you think you could find someone to write some testcases (in form > of mochitest), perhaps based on the requirements from FFOS? Unfortunately I couldn't find someone at this point...perhaps Jet may know if someone can back Mats up? Or we need to wait until Mats is back..Thanks.
Flags: needinfo?(hochang) → needinfo?(bugs)
Comment on attachment 8456828 [details] [diff] [review] part 1, Exclude non-selectable nodes from the Selection. >+nsRange::ExcludeNonSelectableNodes(nsTArray<nsRefPtr<nsRange>>* aOutRanges) I'm having trouble to understand this method since... >+{ >+ MOZ_ASSERT(mIsPositioned); >+ if (Collapsed()) { >+ nsCOMPtr<nsIContent> start = do_QueryInterface(mStartParent); >+ bool selectable = true; >+ if (start && start->GetPrimaryFrame()) { >+ start->GetPrimaryFrame()->IsSelectable(&selectable, nullptr); >+ } >+ if (selectable) { >+ aOutRanges->AppendElement(this); >+ } here you append 'this' to the results and start is selectable. But perhaps you should meant !selectable, like below Also, in general, if an nsIContent doesn't have primary frame, shouldn't we check its ancestors to find a node which has primary frame and check its selectability. >+ } else if (firstNonSelectableContent) { >+ if (range == this && !seenSelectable) { >+ // This is the first range and all its nodes uptil now are until >+ // If aParentNode is inside a range in a multi-range selection we need >+ // to remove the ranges that follows in the selection direction and >+ // make that range the mAnchorFocusRange. Don't really understand this stuff either.
Attachment #8456828 - Flags: review?(bugs)
Attachment #8456828 - Flags: review-
Attachment #8456828 - Flags: feedback?(mats)
(In reply to howie [:howie] from comment #29) > (In reply to Olli Pettay [:smaug] from comment #26) > > howie, do you think you could find someone to write some testcases (in form > > of mochitest), perhaps based on the requirements from FFOS? > > Unfortunately I couldn't find someone at this point...perhaps Jet may know > if someone can back Mats up? Or we need to wait until Mats is back..Thanks. This patch just got an r- and Mats will need to repair it.
Flags: needinfo?(bugs)
Hi Mats, are you back from the vacation? :-) Any chance you could help on the last part of this? Thank you!
Flags: needinfo?(mats)
(In reply to Olli Pettay [:smaug] from comment #27) > >+ // use nsCopySupport::etSelectionForCopy() ? > s/et/Get/ Fixed.
Attachment #8478357 - Flags: review+
Attachment #8456819 - Attachment is obsolete: true
Flags: needinfo?(mats)
Attachment #8456828 - Attachment is obsolete: true
Attachment #8456828 - Flags: feedback?(mats)
Comment on attachment 8478359 [details] [diff] [review] part 1, (WIP) Exclude non-selectable nodes from the Selection. (In reply to Olli Pettay [:smaug] from comment #30) FYI, these patches were mostly just proof-of-concept and not really ready for review. > >+nsRange::ExcludeNonSelectableNodes(nsTArray<nsRefPtr<nsRange>>* aOutRanges) > I'm having trouble to understand this method since... > > >+{ > >+ MOZ_ASSERT(mIsPositioned); > >+ if (Collapsed()) { > >+ nsCOMPtr<nsIContent> start = do_QueryInterface(mStartParent); > >+ bool selectable = true; > >+ if (start && start->GetPrimaryFrame()) { > >+ start->GetPrimaryFrame()->IsSelectable(&selectable, nullptr); > >+ } > >+ if (selectable) { > >+ aOutRanges->AppendElement(this); > >+ } > here you append 'this' to the results and start is selectable. > But perhaps you should meant !selectable, like below This first block just deals with a Collapsed() range and does an early return - I'm not sure why I kept it since the result should be the same without it. We can remove it I think, but feel free to run through the algorithm in your head with a collapsed range on a) a selectable node - should result in that range being put in aOutRanges unmodified; b) a non-selectable node - should result in aOutRanges being empty. (We might want to add an assertion to that effect.) > Also, in general, if an nsIContent doesn't have primary frame, shouldn't we > check its ancestors > to find a node which has primary frame and check its selectability. Seems reasonable. (We do have a style context for the root of the display:none subtree though (in nsFrameManager) in case that's useful for something.) > >+ } else if (firstNonSelectableContent) { > >+ if (range == this && !seenSelectable) { > >+ // This is the first range and all its nodes uptil now are > until Fixed. > >+ // If aParentNode is inside a range in a multi-range selection we need > >+ // to remove the ranges that follows in the selection direction and > >+ // make that range the mAnchorFocusRange. > Don't really understand this stuff either. Example: data:text/html,hello<p style="-moz-user-select: none;">unselectable</p>world Drag-select from "e" to make the selection "elloworl". That creates a selection with two ranges, the latter being the mAnchorFocusRange (it indicates which range should draw the caret on platforms where it's visible). Now Shift-click on "l" in "hello". (The expected result is a selection with one range: "hel".) Selection::Extend operates on mAnchorFocusRange and it would lead to a rather surprising result unless we first set mAnchorFocusRange to the range that we Shift-clicked on, and remove any ranges after it. That's what this added block of code does.
Comment on attachment 8478359 [details] [diff] [review] part 1, (WIP) Exclude non-selectable nodes from the Selection. nsRange::ExcludeNonSelectableNodes doesn't exclude <p> from being selected in this example: data:text/html,hello<span><p style="-moz-user-select: none;">unselectable</p></span>world so I believe RangeSubtreeIterator is probably the wrong iterator to use in this algorithm (it never visits <p>). So this method needs more work...
Attachment #8478359 - Flags: review-
Before spending time on fixing the remaining issues, it would be good to get confirmation that this stuff is actually going to be used in FFOS. Bug 1037286 comment 4 adds some doubt about that. There's also the spec issue around multi-range Selection mentioned in comment 9. We don't want the suggested solution in "part 1" if the DOM Selection spec forbids more than one Range in the Selection. That's something for DOM people to sort out I think.
Flags: needinfo?(bugs)
Yes, based on current discussion and according to page 3 of latest UX spec https://bug1037286.bugzilla.mozilla.org/attachment.cgi?id=8478903 , this is a fundamental fix for FF OS to move on non-editable copy paste, since per app needs to define each element is selectable or not by moz-user-select and UX is good with this approach. Adding UX Omega into cc list. Thanks.
Attachment #8478359 - Attachment is obsolete: true
Flags: needinfo?(bugs)
Comment on attachment 8481919 [details] [diff] [review] part 1, Exclude non-selectable nodes from the Selection. >+ nsINode* node = iter->GetCurrentNode(); >+ iter->Next(); >+ bool selectable = true; >+ nsCOMPtr<nsIContent> content = do_QueryInterface(node); nsIContent* content = node && node->IsContent ? node->AsContent() : nullptr; >+ } else { >+ // Save the end point before truncating the range. >+ auto endParent = range->mEndParent; >+ auto endOffset = range->mEndOffset; I'm not really a fan of auto. It makes writing code faster but tend to make reading harder. So please use the real types here, pretty please. >+ struct MOZ_STACK_CLASS AutoApplyUserSelectStyle { { goes to its own line >+ nsresult AddItemInternal(nsRange *aRange, int32_t* aOutIndex); Nit, * goes with type. So nsRange* >+ bool IsUserSelectionReason() const >+ { >+ return mSelectionChangeReason != nsISelectionListener::NO_REASON; >+ } I'd filter out also COLLAPSETOSTART_REASON and COLLAPSETOEND_REASON >+ Maybe<Selection::AutoApplyUserSelectStyle> userSelect; >+ if (IsUserSelectionReason()) { >+ userSelect.emplace(mDomSelections[index]); >+ } Do we need AutoApplyUserSelectStyle if aContinueSelection is false? In other words, could we move the Maybe stuff to be in the 'else' after 'if (!aContinueSelection) {' >+ nsDirection dir = GetDirection(); >+ >+ // If aParentNode is inside a range in a multi-range selection we need >+ // to remove the ranges that follows in the selection direction and >+ // make that range the mAnchorFocusRange. This algorithm is rather slow, but hopefully rarely needed. Some tests are needed.
Attachment #8481919 - Flags: review?(bugs) → review+
Attached patch part 2, tests — — Splinter Review
(In reply to Olli Pettay [:smaug] from comment #40) > >+ nsCOMPtr<nsIContent> content = do_QueryInterface(node); > nsIContent* content = node && node->IsContent ? node->AsContent() : nullptr; Fixed. (Could we have a static method on nsIContent that does this boilerplate, please?) (/me mutters about our anachronistic 80-column rule...) > >+ auto endParent = range->mEndParent; > >+ auto endOffset = range->mEndOffset; > So please use the real types here, pretty please. Fixed. > >+ struct MOZ_STACK_CLASS AutoApplyUserSelectStyle { > { goes to its own line Fixed. > >+ nsresult AddItemInternal(nsRange *aRange, int32_t* aOutIndex); > Nit, * goes with type. So nsRange* Oops, I copy-pasted AddItem. Fixed both. > >+ bool IsUserSelectionReason() const > >+ { > >+ return mSelectionChangeReason != nsISelectionListener::NO_REASON; > >+ } > I'd filter out also > COLLAPSETOSTART_REASON and COLLAPSETOEND_REASON Yeah, I looked into how these reason codes are used more closely and found that SELECTALL_REASON can also be used for script. So I inverted the check so it looks for MOUSE/KBD/DRAG specifically. > >+ Maybe<Selection::AutoApplyUserSelectStyle> userSelect; > >+ if (IsUserSelectionReason()) { > >+ userSelect.emplace(mDomSelections[index]); > >+ } > Do we need AutoApplyUserSelectStyle if aContinueSelection is false? aContinueSelection is false also for Ctrl+click and I think we might need it there. The TakeFocus method calls Extend also when aContinueSelection is false in some cases. > >+ // If aParentNode is inside a range in a multi-range selection we need > >+ // to remove the ranges that follows in the selection direction and > >+ // make that range the mAnchorFocusRange. > This algorithm is rather slow, but hopefully rarely needed. It should only run for user gestures, not script, so I think it should be OK. There are ways to optimize this traversal if we find it's needed. > Some tests are needed. Fixed. https://tbpl.mozilla.org/?tree=Try&rev=dc301974fe74 https://tbpl.mozilla.org/?tree=Try&rev=b1ba440f2188
Attachment #8481919 - Attachment is obsolete: true
Attachment #8485985 - Flags: review?(bugs)
Comment on attachment 8485985 [details] [diff] [review] part 1, Exclude non-selectable nodes from the Selection. Please test ctrl+a performance in cases like html spec http://www.whatwg.org/specs/web-apps/current-work/ or tbpl logs. Please document that ExcludeNonSelectableNodes may change the range itself, or may not.
Attachment #8485985 - Flags: review?(bugs) → review+
single page HTML5 spec: Linux64 debug build, CTRL+A without patch: ~7 sec, with patch: ~7 sec. OSX Opt shark build, CTRL+A with patch: nsRange::ExcludeNonSelectableNodes takes 5ms (0.6%). (This test is completely dominated by painting.) tbpl Full Log from a Linux debug reftest run: Linux64 debug build, CTRL+A without patch: ~1.4 sec, with patch: ~1.4 sec. OSX Opt shark build, CTRL+A with patch: nsRange::ExcludeNonSelectableNodes doesn't appear in the list, i.e. it's less than 0.2%. (This test is dominated by string and nsPlainTextSerializer operations)
(In reply to Olli Pettay [:smaug] from comment #43) > Please document that ExcludeNonSelectableNodes may change the range itself, > or may not. /** * Scan this range for -moz-user-select:none nodes and split it up into * multiple ranges to exclude those nodes. The resulting ranges are put * in aOutRanges. If no -moz-user-select:none node is found in the range * then |this| is unmodified and is the only range in aOutRanges. * Otherwise, |this| will be modified so that it ends before the first * -moz-user-select:none node and additional ranges may also be created. * If all nodes in the range are -moz-user-select:none then aOutRanges * will be empty. * @param aOutRanges the resulting set of ranges */ void ExcludeNonSelectableNodes(nsTArray<nsRefPtr<nsRange>>* aOutRanges);
Attachment #8485985 - Attachment is obsolete: true
Attachment #8486119 - Flags: review+
Backed out for Mulet perma-fail. I tried to find you on IRC to see about just disabling the test there, but was unsuccessful. https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f19bab6f43 https://tbpl.mozilla.org/php/getParsedLog.php?id=47785131&tree=Mozilla-Inbound
Thank you Mats! \o/
Blocks: 1089184
Depends on: 1123067
Depends on: 1123505
Depends on: 1129078
Depends on: 1128722
Depends on: 1134588
No longer depends on: 1134588
Depends on: 1216001
Depends on: 1488717
Regressions: 1625534
Regressions: 1730180
Regressions: 1861565
Regressions: 1976237
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: