Closed Bug 1062735 Opened 5 years ago Closed 5 years ago

[Text Selection] Enabling non-editable fields support for selection caret

Categories

(Core :: Selection, defect, P1)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: mtseng, Assigned: mtseng)

References

Details

Attachments

(5 files, 10 obsolete files)

3.65 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.64 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
271 bytes, text/html
Details
16.90 KB, patch
Details | Diff | Splinter Review
No description provided.
Attached patch (wip)non-editable (obsolete) — Splinter Review
WIP patch for supporting non-editable selection.
Priority: -- → P1
See Also: → 1065142
Can you please share what you're trying to achieve in this bug?  Thanks!
Flags: needinfo?(mtseng)
Sure, the implementation of selection caret right now rely on caret. That is, we only support editable fields now because caret only appear on editable fields. So what I trying to do is removing all logic which rely on caret. Thus, we can long tap on non-editable fields to select contents and carets would appear just like selection caret on editable fields now.
Flags: needinfo?(mtseng)
Summary: [Text Selection] Enabling non-editable field support → [Text Selection] Enabling non-editable fields support for selection caret
Our caret works fine with non-editable content.  That is how the "caret browsing mode" is implemented.  To try it out on desktop, for example, check the "Always use the cursor keys to navigate within pages" item from the Advanced panel of the Preferences window.

Is bug 1065142 comment 2 not enough to fix this?
Flags: needinfo?(mtseng)
I think bug 1065142 comment 2 is not enough to fix. For example, [1] get frame selection from caret frame. If we do not enable "caret browsing mode" then we cannot get frame selection in non-editable fields. There are more logic bound to caret like this. So this bug would remove all logic like this exmpale so that we can support both editable and non-editable fields.


[1]: http://mxr.mozilla.org/mozilla-central/source/layout/base/SelectionCarets.cpp#430
Flags: needinfo?(mtseng)
I think we should enable caret browsing mode here.  But if you have a better solution which works as well, then I'd be curious to know!
Attachment #8484039 - Attachment is obsolete: true
Attachment #8495729 - Flags: review?(roc)
Attached patch Part 2: Add preference (obsolete) — Splinter Review
Add preference for non-editable support.
Attachment #8495730 - Flags: review?(roc)
Basic idea to support non-editable fields here is removing caret dependency because non-editable fields doesn't have caret unless you turn "browserWithCaret" on. In order to remove caret dependency, I get focusedContent from nsFocusManager and use selection information of this content to show selection carets.
Attachment #8495731 - Flags: review?(roc)
Comment on attachment 8495729 [details] [diff] [review]
Part 1: Remove GetHintForPosition

Review of attachment 8495729 [details] [diff] [review]:
-----------------------------------------------------------------

Please explain why you're removing this.
Attachment #8495729 - Flags: review?(roc) → review-
Comment on attachment 8495730 [details] [diff] [review]
Part 2: Add preference

Review of attachment 8495730 [details] [diff] [review]:
-----------------------------------------------------------------

Please explain why we need a pref to control this.
Attachment #8495730 - Flags: review?(roc) → review-
Comment on attachment 8495731 [details] [diff] [review]
Part 3: Support non-editable fields

Review of attachment 8495731 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/SelectionCarets.cpp
@@ +305,2 @@
>    }
> +  aRect.width = AppUnitsPerCSSPixel();

This change should go in its own patch. (It's a good change though!)

@@ -417,5 @@
> -    nsRect rect = frame->GetRectRelativeToSelf();
> -    nsLayoutUtils::TransformRect(frame, rootFrame, rect);
> -    resultRect = resultRect.Union(rect);
> -  }
> -

Why are you removing this?

@@ +448,5 @@
> +      nsLayoutUtils::IGNORE_CROSS_DOC |
> +      nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME);
> +
> +  SetStartFrameVisibility(hitFramesInFirstRect.Contains(startFrame));
> +  SetEndFrameVisibility(hitFramesInLastRect.Contains(endFrame));

This change doesn't look related. Can you separate it to its own patch?

@@ +495,5 @@
>  
>    SetCaretDirection(mPresShell->GetSelectionCaretsStartElement(), startFrameIsRTL);
>    SetCaretDirection(mPresShell->GetSelectionCaretsEndElement(), !endFrameIsRTL);
>    SetTilted(isTilt);
> +  printf_stderr("Morris UpdateSelectionCarets\n");

Remove this
Attachment #8495731 - Flags: review?(roc) → review-
The purpose of GetHintForPosition is returning CARET_ASSOCIATE_AFTER once we hit newline character. But this doesn't match my requirements. For example, a textarea with content "abcde" and width 20px which is not wide enough to contain "abcde" in one line. So GetHintForPosition cannot detect line break for such situation. And then for the selection range with end node at end of line, I want to get CARET_ASSOCIATE_BEFORE frame instead of after frame. So for those two reasons I cannot use GetHintForPosition. Then I found return CARET_ASSOCIATE_AFTER for begin of selection range and CARET_ASSOCIATE_BEFORE for end of selection range seems match my requirement. So I could remove GetHintForPosition.
Attachment #8495729 - Attachment is obsolete: true
Attachment #8496702 - Flags: review?(roc)
We have stable state for editable fields selection now. And we don't want to break this stable state when enabling non-editable support. So I add a pref to control to support or not. Once non-editable fields support is stable. We should remove this pref.
Attachment #8495730 - Attachment is obsolete: true
Attachment #8496703 - Flags: review?(roc)
Fix nit
Attachment #8496705 - Flags: review?(roc)
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (on vacation Sep 27 to Oct 12) from comment #12)
> Comment on attachment 8495731 [details] [diff] [review]
> Part 3: Support non-editable fields
> 
> Review of attachment 8495731 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/SelectionCarets.cpp
> @@ +305,2 @@
> >    }
> > +  aRect.width = AppUnitsPerCSSPixel();
> 
> This change should go in its own patch. (It's a good change though!)
Done.
> 
> @@ -417,5 @@
> > -    nsRect rect = frame->GetRectRelativeToSelf();
> > -    nsLayoutUtils::TransformRect(frame, rootFrame, rect);
> > -    resultRect = resultRect.Union(rect);
> > -  }
> > -
> 
> Why are you removing this?
Explain below.
> 
> @@ +448,5 @@
> > +      nsLayoutUtils::IGNORE_CROSS_DOC |
> > +      nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME);
> > +
> > +  SetStartFrameVisibility(hitFramesInFirstRect.Contains(startFrame));
> > +  SetEndFrameVisibility(hitFramesInLastRect.Contains(endFrame));
> 
> This change doesn't look related. Can you separate it to its own patch?
In the non-editable support I cannot find a general way to detect visibility just like the code I removed at above comment. So I found that I can use GetFramesForArea to detect visibility for both editable and non-editable fields without to much complex code. So I think this change still related to non-editable fields support and I still put it to the same patch.
> 
> @@ +495,5 @@
> >  
> >    SetCaretDirection(mPresShell->GetSelectionCaretsStartElement(), startFrameIsRTL);
> >    SetCaretDirection(mPresShell->GetSelectionCaretsEndElement(), !endFrameIsRTL);
> >    SetTilted(isTilt);
> > +  printf_stderr("Morris UpdateSelectionCarets\n");
> 
> Remove this
Done.
Comment on attachment 8496705 [details] [diff] [review]
Part 4: Support non-editable fields

Review of attachment 8496705 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/SelectionCarets.cpp
@@ +850,2 @@
>  {
> +  nsCOMPtr<nsIContent> focusNode = GetFocusedContent();

I think nsIContent* is sufficient.
Fix nit addressed by TYLin.
Attachment #8496705 - Attachment is obsolete: true
Attachment #8496705 - Flags: review?(roc)
Attachment #8501504 - Flags: review?(roc)
Comment on attachment 8501504 [details] [diff] [review]
Part 4: Support non-editable fields v2

Review of attachment 8501504 [details] [diff] [review]:
-----------------------------------------------------------------

r- only because of the last comment.

::: layout/base/SelectionCarets.cpp
@@ +455,5 @@
> +      nsLayoutUtils::IGNORE_CROSS_DOC |
> +      nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME);
> +
> +  SetStartFrameVisibility(hitFramesInFirstRect.Contains(startFrame));
> +  SetEndFrameVisibility(hitFramesInLastRect.Contains(endFrame));

The existing comment "// Check if caret inside the scroll frame's boundary" is confusing because the code doesn't do that. In the code we just find the editing host element and clip to its border-box(es). That's not a good thing to do in general, though it's OK for form controls. For example, if the whole document is editable and it contains a scrollable DIV, and the selection carets are inside the DIV but scrolled out of view, I think they will still be considered visible as long as they're in the document's viewport.

So, although there isn't a great solution to the visibility issues we're trying to solve here, the approach you've chosen is actually pretty good. Well done :-).

@@ +534,5 @@
>    nsLayoutUtils::TransformPoint(canvasFrame, ptFrame, ptInFrame);
>  
> +  // If target frame is editable, we should move focus to targe frame. If
> +  // target frame isn't editable and our focus content is editable, we should
> +  // clear focus.

Why can't we just focus the non-editable target? It's fine for non-editable content to be focused.
Attachment #8501504 - Flags: review?(roc) → review-
Attached file test_focus.html
Test focus for non-editable field.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #20)
> Comment on attachment 8501504 [details] [diff] [review]
> Part 4: Support non-editable fields v2
> 
> Review of attachment 8501504 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +534,5 @@
> >    nsLayoutUtils::TransformPoint(canvasFrame, ptFrame, ptInFrame);
> >  
> > +  // If target frame is editable, we should move focus to targe frame. If
> > +  // target frame isn't editable and our focus content is editable, we should
> > +  // clear focus.
> 
> Why can't we just focus the non-editable target? It's fine for non-editable
> content to be focused.
I found if I focus non-editable field, the focus still in the editable field. Please see the above attachment. So I think focus non-editable field is not working here. roc, do you have other suggestion? Thanks.
Flags: needinfo?(roc)
Comment on attachment 8501504 [details] [diff] [review]
Part 4: Support non-editable fields v2

Review of attachment 8501504 [details] [diff] [review]:
-----------------------------------------------------------------

You're right!
Attachment #8501504 - Flags: review- → review+
Hi, roc
There are some bugs in previous patch. So I did slightly changes in this patch. Please review it again for those changes, thanks.

First change is I removed selection change reason filter in NotifySelectionChanged. In previous patch, I only call UpdateSelectionCarets when the reason is mouseup or selectall. This code would cause carets remain on screen after cutting text. So I remove it in this patch.

Second change is add IsSelectable check in DragSelection function. If we don't do this check. There is weird behavior if we drag over the non-selectable element. nsFrame do similar check here [1]

[1]: http://dxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp#2745
Attachment #8501504 - Attachment is obsolete: true
Attachment #8505226 - Flags: review?(roc)
Flags: needinfo?(roc)
Fix build break for non-unified build.
Attachment #8505226 - Attachment is obsolete: true
Attachment #8505226 - Flags: review?(roc)
Attachment #8505351 - Flags: review?(roc)
Fix try server fail.
Attachment #8505351 - Attachment is obsolete: true
Attachment #8505351 - Flags: review?(roc)
Attachment #8506055 - Flags: review?(roc)
Fix try failure which show selection carets when non-editable pref is off. So I add a check for content is editable when showing selection carets.

Latest try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8946ab3ccdd3
Attachment #8506055 - Attachment is obsolete: true
Attachment #8506055 - Flags: review?(roc)
Attachment #8506126 - Flags: review?(roc)
You need to log in before you can comment on or make changes to this bug.