[Webkit-unassigned] [Bug 91912] Adding "all" to -webkit-user-select

Tue Oct 30 17:11:42 PDT 2012

https://bugs.webkit.org/show_bug.cgi?id=91912

--- Comment #76 from Ryosuke Niwa <rniwa at webkit.org>  2012-10-30 17:12:57 PST ---
(From update of attachment 171550)
View in context: https://bugs.webkit.org/attachment.cgi?id=171550&action=review

> Source/WebCore/dom/Node.h:381
> +    enum ContentEditableUse {
> +        APIUse,
> +        InternalUse
> +    };

I'm not loving the enum & value names here. How about something like
enum ShouldTreatSelectAllAsEditable {
   TreatSelectAllAsEditable,
   DoNoTreatSelectAllAsEditable,
}

> Source/WebCore/dom/Position.cpp:875
> +#if ENABLE(USERSELECT_ALL)
> +    return node && node->renderer() && node->renderer()->style()->userSelect() == SELECT_ALL;
> +#else
> +    UNUSED_PARAM(node);
> +    return false;
> +#endif

I would prefer putting this if-def in the header file so that we can compiler-time eliminate the member functions altogether on ports/platforms where this feature is not enabled to avoid any perf. hits.

> Source/WebCore/editing/FrameSelection.cpp:599
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)

You should define Node* inside the if.

> Source/WebCore/editing/FrameSelection.cpp:645
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)
> +        pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionAfterNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionBeforeNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> +#endif    

It seems like these code blocks are identical. Can we extract a helper function so that we don't have to duplicate it everywhere?

> Source/WebCore/editing/FrameSelection.cpp:776
> +#if ENABLE(USERSELECT_ALL)
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)
> +        pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionBeforeNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionAfterNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> +#endif

Ditto.

> Source/WebCore/editing/FrameSelection.cpp:825
> +#if ENABLE(USERSELECT_ALL)
> +    Node* rootUserSelectAll = Position::rootUserSelectAllForNode(pos.deepEquivalent().anchorNode());
> +    if (rootUserSelectAll)
> +        pos = VisiblePosition((directionOfEnclosingBlock() == LTR) ? positionBeforeNode(rootUserSelectAll).downstream(CanCrossEditingBoundary) : positionAfterNode(rootUserSelectAll).upstream(CanCrossEditingBoundary));
> +#endif

Ditto.

> LayoutTests/editing/selection/user-select-all-selection-expected.txt:31
> +

Can we add a test case to set the selection programmatically?
e.g. if we had <html><head></head><body><div>hello, <span style="-moz-user-select: all;">wor<b>l</b>d</span> WebKit</div></body></html>, Mozilla still lets us select just b by running getSelection().selectAllChildren(document.querySelector('b'));

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.