196774 - Can insert caret into -moz-user-select:all subtree that wraps across several lines

Status: NEW

(Core :: DOM: Selection, defect)

Description

[kinmoz]

If you have a -moz-user-select:all subtree that wraps and spans 2 or more lines,
you can actually place the caret into the subtree by clicking to the right
(roughly where the y coordinate of the baseline would be) of the 1st line
containing the subtree.

This totally freaks out the editor. Any text you enter at that point will appear
on the last line that the subtree wraps to, and even if you've typed several
characters, a single backspace over any char you just entered will result in the
entire -moz-user-select:all subtree disappearing.

Comment 1

[Juan José Mata]

Nominating topembed since this is needed to complete the atomic selection
feature delivered to one of our embedding clients.

Comment 2

[mjudge]

yes i know about this. it has to do with jumping outside of a link before it
jumps outside the atomic selected stuff. I will have a fix posted by end of the day.

Comment 3

[mjudge]

Attachment: patch for nsFrame.cpp

this does a check to make sure we dont go into the link specific code.	If we
dont do this check then the link code detects that its in a special link case
and "fixes" selection and leaves before the display all code can be triggered.

Comment 4

[mjudge]

Attachment: patch for nsFrame.cpp

this bigger patch also fixes home-end issues. I just realized you cant have the
previous patch work without this code as well.	I will also add an nsBlockFrame
patch.

Comment 5

[mjudge]

Attachment: patch for nsBlockFrame.cpp

small patch to have the handleevent code abort when the getprevline call
triggers that it hit an inline stop (for links)

Comment 6

[mjudge]

Attachment: patch for nsFrame.cpp

combines up/down/home/end/left/right into one check for USER_SELECT_ALL this
saved space and also irregularities.  if peek offset ends up in atomic
selection, continue until you are no longer in an atomic selection.

Comment 7

[mjudge]

Attachment: nsTextFrame.dif

use this in addition to the nsFrame code.  This will prevent accidental carat
movement into the select all style.  This also uses a common place to catch all
the edge cases.

Comment 8

[Juan José Mata]

When is this landing on the trunk?  Also, are the patches available ready to
land on the ANGELON branch?

Comment 9

[mjudge]

Attachment: inclusive patch

path from top of layout. this should be up to date

Comment 10

[Charles Rosendahl]

20030327 Trunk build:

I have noticed that you can arrow around the bounds of the span object and enter
text within the moz-user-select:all subtree as well.

In particular, after a drop of the subtree, the cursor is placed to the right of
the element after the anchor element but before the closing </span> tag.

Comment 11

[kinmoz]

Comment on attachment 117658 [details] [diff] [review]
inclusive patch

It looks like this patch can cause us to blow the runtime stack in the case
where  the last thing in the document is a -moz-user-select:all subtree, and
you click below it in the empty content area of the body.

The problem seems to be that when we click below all of the content, the event
gets dispatched through the body frame's nsBlockFrame::HandleEvent() method,
which then calls nsFrame::GetNextPrevLineFromeBlockFrame() which returns a
pos.mResultFrame pointer that points at the body frame, so we effectively end
up
calling pos.mResultFrame->HandleEvent() later in the method causing us to
infinitely recurse.

The problem seems to be that GetNextPrevLineFromeBlockFrame() initially
calculates an pos.mResultFrame that is inside a -moz-user-select:all subtree so
then it executes this code that was added:

+    do 
+    {
+      currentFrame = parentFrame;
+      currentFrame->GetParent(&parentFrame);
+      if (parentFrame)
+	 parentFrame->IsSelectable(&selectable, &parentStyle);
+    }
+    while (parentFrame && (parentStyle == NS_STYLE_USER_SELECT_ALL));


which ends up crawling up the frame hierarchy until we hit the body again. The
code after this then resets pos.mResultFrame to parentFrame and returns.

Comment 12

[mjudge]

if any branch needs this earlier I can check the code in as well to 1.4b/final.
 This will take a little time to do right and I dont want to cause a regression
before vacation.

Comment 13

[chris hofmann]

any other owners for this?
unsetting target milestone

Comment 14

[Uri Bernstein]

Eli, you might be interested in this. It seems like your patch to bug 316281 somewhat improved the situation here (typing characters adds them at the end of the -moz-user-select:all element, and they can be deleted normally), but the basic issue is still there.

Comment 15

[Eli Friedman]

The "basic issue" being that caret drawing doesn't account for inline continuations?  That seems relatively easy to fix by checking for continuations before drawing the caret for a non-text frame (the solution in the patch for this bug doesn't look like it would work correctly).

There's also the issue of bugs related to keyboard navigation and dragging -moz-user-select-all trees.  The patch bug 316281 actually prevents up/down from going into -moz-user-select-all trees, although it breaks getting past them.  I think the best approach to fixing the many issues related to -moz-user-select: all and the keyboard would basically be cleaning up PeekOffset (I forget the bug number).  I'm not quite sure what the drag path does; that would take a bit of research.

Skimming the patch, it doesn't look useful, and it looks like it introduces some bugs, although I could be wrong.

I think it would be best to split the caret drawing issue off into another bug, and do the actual work in other bugs.  This would then become a sort of tracker for the issues brought up in this bug so we don't lose track of the problems.  Should I go ahead and do that, or do you have a different idea?

Comment 16

[Stephen Donner [:stephend] Not actively reading bugmail]

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months.
:hsinyi, could you have a look please?
For more information, please visit auto_nag documentation.