1195672 - Revise AccessibleCaret's long-press logic

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

[Ting-Yu Lin [:TYLin] (UTC-8)]

The long-press logic of AccessibleCaret has some bugs. One of them is the user cannot reliably long-press a empty content to bring up the text selection dialog as reported in [1].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1190980#c0

Comment 1

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: MozReview Request: Bug 1195672 - Add |nsAutoCString nsIFrame::ListTag()| for debugging. f=mtseng, r=roc

Bug 1195672 - Add |nsAutoCString nsIFrame::ListTag()| for debugging. f=mtseng, r=roc

This make it easier to print a frame tag name in one debug log line.

Comment 2

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: MozReview Request: Bug 1195672 - Make focus changing by long tap behaves like by single tap. f=mtseng, r=roc

Bug 1195672 - Make focus changing by long tap behaves like by single tap. f=mtseng, r=roc

We want the focus changing behavior by long tap as close as to the one
by single tap.

The only functional change is that we always clear old focus and
re-focus the window if a focusable frame cannot be found. This behavior
is the same as the single tap implemented in
EventStateManager::PostHandleEvent().

Besides, ChangeFocus now returns the new focusable frame instead of bool
which provides more information.

Comment 3

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: MozReview Request: Bug 1195672 - Move the check that frame is selectable into SelectWord. f=mtseng, r=roc

Bug 1195672 - Move the check that frame is selectable into SelectWord. f=mtseng, r=roc

There's a bug that when a frame is focusable but not selectable, we
won't focus on it because we call IsSelectable() before ChangeFocus().

By moving the check into SelectWord(), we'll have a chance to focus on
it.

This resolves a issue that when long press to select a word on a new
opened app, the selection highlight is gray instead of blue.

Comment 4

[Ting-Yu Lin [:TYLin] (UTC-8)]

Attachment: MozReview Request: Bug 1195672 - Revise the logic of long tap on empty content. f=mtseng, r=roc

Bug 1195672 - Revise the logic of long tap on empty content. f=mtseng, r=roc

The only logic change is that we now call UpdateCaret() before
dispatching CaretStateChangedEvent.

This resolves a bug that the text selection dialog flashes when long
tapping on an empty content.

Comment 5

[Ting-Yu Lin [:TYLin] (UTC-8)]

Comment on attachment 8649728 [details]
MozReview Request: Bug 1195672 - Add |nsAutoCString nsIFrame::ListTag()| for debugging. f=mtseng, r=roc

Bug 1195672 - Add |nsAutoCString nsIFrame::ListTag()| for debugging. f=mtseng, r=roc

This make it easier to print a frame tag name in one debug log line.

Comment 6

[Ting-Yu Lin [:TYLin] (UTC-8)]

Comment on attachment 8649729 [details]
MozReview Request: Bug 1195672 - Make focus changing by long tap behaves like by single tap. f=mtseng, r=roc

Bug 1195672 - Make focus changing by long tap behaves like by single tap. f=mtseng, r=roc

We want the focus changing behavior by long tap as close as to the one
by single tap.

The only functional change is that we always clear old focus and
re-focus the window if a focusable frame cannot be found. This behavior
is the same as the single tap implemented in
EventStateManager::PostHandleEvent().

Besides, ChangeFocus now returns the new focusable frame instead of bool
which provides more information.

Comment 7

[Ting-Yu Lin [:TYLin] (UTC-8)]

Comment on attachment 8649730 [details]
MozReview Request: Bug 1195672 - Move the check that frame is selectable into SelectWord. f=mtseng, r=roc

Bug 1195672 - Move the check that frame is selectable into SelectWord. f=mtseng, r=roc

There's a bug that when a frame is focusable but not selectable, we
won't focus on it because we call IsSelectable() before ChangeFocus().

By moving the check into SelectWord(), we'll have a chance to focus on
it.

This resolves a issue that when long press to select a word on a new
opened app, the selection highlight is gray instead of blue.

Comment 8

[Ting-Yu Lin [:TYLin] (UTC-8)]

Comment on attachment 8649731 [details]
MozReview Request: Bug 1195672 - Revise the logic of long tap on empty content. f=mtseng, r=roc

Bug 1195672 - Revise the logic of long tap on empty content. f=mtseng, r=roc

The only logic change is that we now call UpdateCaret() before
dispatching CaretStateChangedEvent.

This resolves a bug that the text selection dialog flashes when long
tapping on an empty content.

Comment 9

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8649728 [details]
MozReview Request: Bug 1195672 - Add |nsAutoCString nsIFrame::ListTag()| for debugging. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16481/#review14765

Ship It!

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8649729 [details]
MozReview Request: Bug 1195672 - Make focus changing by long tap behaves like by single tap. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16483/#review14767

Ship It!

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8649730 [details]
MozReview Request: Bug 1195672 - Move the check that frame is selectable into SelectWord. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16485/#review14769

Ship It!

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8649731 [details]
MozReview Request: Bug 1195672 - Revise the logic of long tap on empty content. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16487/#review14771

Ship It!

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8649728 [details]
MozReview Request: Bug 1195672 - Add |nsAutoCString nsIFrame::ListTag()| for debugging. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16481/#review14939

Ship It!

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8649729 [details]
MozReview Request: Bug 1195672 - Make focus changing by long tap behaves like by single tap. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16483/#review14941

Ship It!

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8649730 [details]
MozReview Request: Bug 1195672 - Move the check that frame is selectable into SelectWord. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16485/#review14943

Ship It!

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8649731 [details]
MozReview Request: Bug 1195672 - Revise the logic of long tap on empty content. f=mtseng, r=roc

https://reviewboard.mozilla.org/r/16487/#review14945

Ship It!

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b356dbdf1043
https://hg.mozilla.org/integration/mozilla-inbound/rev/233a48d2d378
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5121bdb277
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab8370de21b7

Comment 18

[Ting-Yu Lin [:TYLin] (UTC-8)]

Morris, roc, thank you for the review.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b356dbdf1043
https://hg.mozilla.org/mozilla-central/rev/233a48d2d378
https://hg.mozilla.org/mozilla-central/rev/6a5121bdb277
https://hg.mozilla.org/mozilla-central/rev/ab8370de21b7