Closed
Bug 1062735
Opened 10 years ago
Closed 10 years ago
[Text Selection] Enabling non-editable fields support for selection caret
Categories
(Core :: DOM: Selection, defect, P1)
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.
Assignee | ||
Updated•10 years ago
|
Blocks: CopyPasteLegacy
Assignee | ||
Comment 1•10 years ago
|
||
WIP patch for supporting non-editable selection.
Updated•10 years ago
|
Priority: -- → P1
Comment 2•10 years ago
|
||
Can you please share what you're trying to achieve in this bug? Thanks!
Flags: needinfo?(mtseng)
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Summary: [Text Selection] Enabling non-editable field support → [Text Selection] Enabling non-editable fields support for selection caret
Comment 4•10 years ago
|
||
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)
Assignee | ||
Comment 5•10 years ago
|
||
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)
Comment 6•10 years ago
|
||
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!
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8484039 -
Attachment is obsolete: true
Attachment #8495729 -
Flags: review?(roc)
Assignee | ||
Comment 8•10 years ago
|
||
Add preference for non-editable support.
Attachment #8495730 -
Flags: review?(roc)
Assignee | ||
Comment 9•10 years ago
|
||
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-
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
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)
Assignee | ||
Comment 15•10 years ago
|
||
Attachment #8495731 -
Attachment is obsolete: true
Attachment #8496704 -
Flags: review?(roc)
Assignee | ||
Comment 17•10 years ago
|
||
(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 18•10 years ago
|
||
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.
Assignee | ||
Comment 19•10 years ago
|
||
Fix nit addressed by TYLin.
Attachment #8496705 -
Attachment is obsolete: true
Attachment #8496705 -
Flags: review?(roc)
Attachment #8501504 -
Flags: review?(roc)
Attachment #8496702 -
Flags: review?(roc) → review+
Attachment #8496703 -
Flags: review?(roc) → review+
Attachment #8496704 -
Flags: review?(roc) → review+
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-
Assignee | ||
Comment 21•10 years ago
|
||
Test focus for non-editable field.
Assignee | ||
Comment 22•10 years ago
|
||
(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+
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Comment 25•10 years ago
|
||
Fix build break for non-unified build.
Attachment #8505226 -
Attachment is obsolete: true
Attachment #8505226 -
Flags: review?(roc)
Attachment #8505351 -
Flags: review?(roc)
Assignee | ||
Comment 26•10 years ago
|
||
Fix try server fail.
Attachment #8505351 -
Attachment is obsolete: true
Attachment #8505351 -
Flags: review?(roc)
Attachment #8506055 -
Flags: review?(roc)
Assignee | ||
Comment 27•10 years ago
|
||
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)
Attachment #8506126 -
Flags: review?(roc) → review+
Assignee | ||
Comment 28•10 years ago
|
||
Fix try fail try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a0b71406e737
Attachment #8506126 -
Attachment is obsolete: true
Comment 30•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/12499d6135bf https://hg.mozilla.org/integration/mozilla-inbound/rev/fe11070e6932 https://hg.mozilla.org/integration/mozilla-inbound/rev/0d49c600a6d5 https://hg.mozilla.org/integration/mozilla-inbound/rev/6a1757feb3ff
Keywords: checkin-needed
Comment 31•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/12499d6135bf https://hg.mozilla.org/mozilla-central/rev/fe11070e6932 https://hg.mozilla.org/mozilla-central/rev/0d49c600a6d5 https://hg.mozilla.org/mozilla-central/rev/6a1757feb3ff
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•