Skip to content

Commit

Permalink
style: Align user-select behavior more with other UAs.
Browse files Browse the repository at this point in the history
There's a few subtle behavior changes here, which I'll try to break down in the
commit message.

The biggest one is the EditableDescendantCount stuff going away. This
was added in bug 1181130, to prevent clicking on the non-editable div from
selecting the editable div inside. This is problematic for multiple reasons:

 * First, I don't think non-editable regions of an editable element should
   be user-select: all.

 * Second, it just doesn't work in Shadow DOM (the editable descendant count is
   not kept up-to-date when not in the uncomposed doc), so nested
   contenteditables behave differently inside vs. outside a Shadow Tree.

 * Third, I think it's user hostile to just entirely disable selection if you
   have a contenteditable descendant as a child of a user-select: all thing.

   WebKit behaves like this patch in the following test-case (though not Blink):

     https://crisal.io/tmp/user-select-all-contenteditable-descendant.html

   Edge doesn't seem to support user-select: all at all (no pun intended).

   But we don't allow to select anything at all which looks wrong.

 * Fourth, it's not tested at all (which explains how we broke it in Shadow DOM
   and not even notice...).

In any case I've verified that this doesn't regress the editor from that bug. If
this regresses anything we can fix it as outlined in the first bullet point
above, which should also make us more compatible with other UAs in that
test-case.

The other change is `all` not overriding everything else. So, something like:

  <div style="-webkit-user-select: all">All <div style="-webkit-user-select: none">None</div></div>

Totally ignores the -webkit-user-select: none declaration in Firefox before this
change. This doesn't match any other UA nor the spec, and this patch aligns us
with WebKit / Blink.

This in turn makes us not need -moz-text anymore, whose only purpose was to
avoid this.

This also fixes a variety of bugs uncovered by the previous changes, like the
SetIgnoreUserModify(false) call in editor being completely useless, since
presShell->SetCaretEnabled ended in nsCaret::SetVisible, which overrode it.

This in turn uncovered even more bugs, from bugs in the caret painting code,
like not checking -moz-user-modify on the right frame if you're the last frame
of a line, to even funnier bits where before this patch you show the caret but
can't write at all...

In any case, the new setup I came up with is that when you're editing (the
selection is focused on an editable node) moving the caret forces it to end up
in an editable node, thus jumping over non-editable ones.

This has the nice effect of not completely disabling selection of
-moz-user-select: all elements that have editable descendants (which was a very
ad-hoc hack for bug 1181130, and somewhat broken per the above), and also
not needing the -moz-user-select: all for non-editable bits in contenteditable.css
at all.

This also fixes issues with br-skipping like not being able to insert content in
the following test-case:

  <div contenteditable="true"><span contenteditable="false">xyz </span><br>editable</div>

If you start moving to the left from the second line, for example.

I think this yields way better behavior in all the relevant test-cases from bug
1181130 / bug 1109968 / bug 1132768, shouldn't cause any regression, and the
complexity is significantly reduced in some places.

There's still some other broken bits that this patch doesn't fix, but I'll file
follow-ups for those.

Differential Revision: https://phabricator.services.mozilla.com/D12687
  • Loading branch information
emilio committed Dec 2, 2018
1 parent 2639916 commit 2e388e8
Show file tree
Hide file tree
Showing 2 changed files with 2 additions and 15 deletions.
1 change: 1 addition & 0 deletions components/style/properties/longhands/ui.mako.rs
Expand Up @@ -39,6 +39,7 @@ ${helpers.predefined_type(
gecko_ffi_name="mUserSelect",
alias="-webkit-user-select",
animation_value_type="discrete",
needs_context=False,
spec="https://drafts.csswg.org/css-ui-4/#propdef-user-select",
)}

Expand Down
16 changes: 1 addition & 15 deletions components/style/values/specified/ui.rs
Expand Up @@ -141,11 +141,6 @@ impl Parse for ScrollbarColor {
}
}

fn in_ua_sheet(context: &ParserContext) -> bool {
use crate::stylesheets::Origin;
context.stylesheet_origin == Origin::UserAgent
}

/// The specified value for the `user-select` property.
///
/// https://drafts.csswg.org/css-ui-4/#propdef-user-select
Expand All @@ -168,15 +163,6 @@ pub enum UserSelect {
Text,
#[parse(aliases = "-moz-none")]
None,
/// Force selection of all children, unless an ancestor has `none` set.
/// Force selection of all children.
All,
/// Like `text`, except that it won't get overridden by ancestors having
/// `all`.
///
/// FIXME(emilio): This only has one use in contenteditable.css, can we find
/// a better way to do this?
///
/// See bug 1181130.
#[parse(condition = "in_ua_sheet")]
MozText,
}

0 comments on commit 2e388e8

Please sign in to comment.