Closed Bug 1101364 Opened 5 years ago Closed 5 years ago

Non-editable element content will be selected even the element already set user-select: none

Categories

(Core :: Selection, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla37
blocking-b2g 2.2+
Tracking Status
firefox35 --- wontfix
firefox36 --- wontfix
firefox37 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: steveck, Assigned: mtseng)

References

Details

Attachments

(3 files, 1 obsolete file)

Reproduce step:
1. Trigger text selection bubble on a non-editable element
2. Click select all

Actual result:
All the content will be select no matter the user-select is none or other.

Expected result:
Element with user-select: none should not be selected

It's the required feature: enable text selection for message bubble in Bug #1092437, so set the dependency for gaia feature.
This it's a critical issue for 2.2 text selection feature in gaia message app bubble selection, nominate blocking flag here.
blocking-b2g: --- → 2.2?
Blocks: CopyPasteLegacy
No longer blocks: 1092437
Blocks: 1092437
Morris, please help to check it
Flags: needinfo?(mtseng)
Priority: -- → P2
Working on this.
Assignee: nobody → mtseng
Flags: needinfo?(mtseng)
Attached file test-user-select.html
Easy html to reproduce this bug.
I found if we have contenteditable elements in dom tree then our select all command would re-direct to nsHTMLEditor::SelectAll() instead of nsDocumentViewer::SelectAll(). So I add AutoApplyUserSelectStyle to nsHTMLEditor::SelectAll().
Attachment #8534874 - Flags: review?(mats)
Comment on attachment 8534874 [details] [diff] [review]
Apply AutoApplyUserSelectStyle to nsHTMLEditor::SelectAll().

This is the right way to do it, but I think we should only do this if the
node is non-editable.  For example, load this in Firefox:

data:text/html,<div contenteditable>AAA<span style="-moz-user-select:none">BBB</span></div>

then click on the text, then CTRL+A (or Select All in the context menu).

Currently it selects all text and you can type DEL to delete all the
text.  With your fix, "BBB" isn't deleted.

In general, things that are editable should ignore 'user-select',
otherwise it's hard to select or position the caret to edit the content.

So I think we should do it something like this:

  Maybe<mozilla::dom::Selection::AutoApplyUserSelectStyle> userSelection;
  if (!rootContent->IsEditable()) {
    userSelection.emplace(selection);
  }

Also, why only fix the !HasIndependentSelection() case?
It seems to me we should do this in both cases.  If you agree, could you
refactor the code a bit to avoid the code repetition please?
In pseudo-code:
if (anchorContent->HasIndependentSelection()) {
  SetAncestorLimiter();
  rootContent = mRootElement;
} else {
  rootContent = GetSelectionRootContent();
}

followed by the rest of the code which can be shared.

(please also remove the code comment as it seems to contradict
 what the code is actually doing)

Also, can you write tests please?  Two reftests should be enough, one for
the editable case and one non-editable.
Attachment #8534874 - Flags: review?(mats) → review-
Update to reviewer's comment.
Attachment #8534874 - Attachment is obsolete: true
Attachment #8535470 - Flags: review?(mats)
Comment on attachment 8535470 [details] [diff] [review]
Part 1: Apply AutoApplyUserSelectStyle to nsHTMLEditor::SelectAll() v2.

Looks good, thanks.  r=mats
Attachment #8535470 - Flags: review?(mats) → review+
Attachment #8535471 - Flags: review?(mats) → review+
https://hg.mozilla.org/mozilla-central/rev/c3df17b883e5
https://hg.mozilla.org/mozilla-central/rev/ee9acb07ca9d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Triage: blocker.
blocking-b2g: 2.2? → 2.2+
You need to log in before you can comment on or make changes to this bug.