1249201 - Continue showing AccessibleCaret when scrolling or panning the page

Status: RESOLVED FIXED

(Core :: DOM: Selection, defect)

Description

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

Currently, when the user is scrolling a page or panning a page, AccessibleCaret will be hidden, and show again until the user action stops. It creates issues like bug 1236434. 

On B2G, the spec requires the current behavior, so it's fine. However, it would be nice if we could keep the carets shows when scrolling the page on Fennec as Google Chrome does. I was wondering if we can do this easily by stopping the normal carets updating process, and asking APZ to pan the carets with the rest of the content.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Well if the carets are position:absolute then they should always be positioned relative to the document and will move with APZ scrolling. I believe they are currently position:fixed which is why APZ doesn't pan them (i.e. they currently use non-normal behaviour, and what you're asking for is the normal behaviour).

That being said, the problem with keep them visible during APZ transformations is that if the user zooms the carets will zoom as well. We could add something that allowed the carets to not zoom although it might be tricky to make the positioning work properly.

Comment 2

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

AccessibleCaret divs are already position:absolute in [1], but they're children of the custom content container under canvas frame, which are position:fixed and possibly in its own layer due to -moz-top-layer: top? I guess that's why the carets get the non-normal behavior.

kats, do you feel it's possible that if AccessibleCaret divs are generated directly in nsCanvasFrame::CreateAnonymousContent() with position:absolute, they can be on the same layer with the content so that APZ can transform them together?

[1] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/layout/style/ua.css#343,346
[2] https://dxr.mozilla.org/mozilla-central/rev/d5daf10d3b74b04e8fa63c4e5429de8a0adf79f8/layout/style/ua.css#434,436-437

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

If using position: absolute is necessary, we can make top layer support that. We need this for <dialog> anyway.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #2)
> kats, do you feel it's possible that if AccessibleCaret divs are generated
> directly in nsCanvasFrame::CreateAnonymousContent() with position:absolute,
> they can be on the same layer with the content so that APZ can transform
> them together?

I would guess yes, but I'm not really sure how that works so I can't say for sure. I don't know what to do about the zooming though, we should figure out how to tackle that before starting implementation work.

Comment 5

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

I hack a prototype [1] to prove that my guess in comment #2 is correct, i.e. carets can use position:absolute so that APZ can transform them together, and carets are continue showing when panning or zooming the page. 

However, like kats said in comment #1, when the user zooms the carets will zoom as well. The size of carets won't be updated to its normal size unless the user lifts his fingers off the screen. 

I cannot tell whether this is already better than the current behavior that carets are hidden during panning or zooming, and can be tackled as a follow-up. Otherwise this is the best I can do on caret code without other modification on APZ.

Anthony, what do you think? If you have time to test this, the build is in [2].

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73b83853755
[2] http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-e73b838537554da7a407cb6b8d9b6cffc5075f09/try-android-api-15/fennec-47.0a1.en-US.android-arm.apk

Comment 6

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

kats, I found nsIScrollObserver::ScrollPositionChanged() is called only when panning (scolling), not when zooming. Do we have a similar callback when zooming?

If we do have a callback during pinch-zooming for updating the carets, I could update only the caret's size since the absolute position of the caret is not changed during pinch-zooming from layout's perspective. Of course the size update of the carets will be lagged since it's updated on content side. But I feel it might be OK.

Comment 7

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #6)
> kats, I found nsIScrollObserver::ScrollPositionChanged() is called only when
> panning (scolling), not when zooming. Do we have a similar callback when
> zooming?
> 

We don't at the moment, but it would be easy to add one. Like you said it would be a bit lagged but it might still be ok. We'd have to try it to see. If you'd like me to write a patch to expose the zoom start/end let me know.

Comment 8

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> We don't at the moment, but it would be easy to add one. Like you said it
> would be a bit lagged but it might still be ok. We'd have to try it to see.
> If you'd like me to write a patch to expose the zoom start/end let me know.

Yes! I'd love to try this if you could help provide such a patch. 

We do already have AsyncPanZoomStarted() and AsyncPanZoomStopped() to notify the APZ transforms for both panning and zooming. Although panning and zooming are not distinguishable from the two callbacks, but this might not be critical for now. What I need is a callback like ZoomLevelChanged() to call repeatedly during zooming, and I'll use [1] to get the current zoom level to scale the caret size.

Currently, in a complete scrolling process, I get:
AsyncPanZoomStarted()
ScrollPositionChanged()
ScrollPositionChanged()
...
AsyncPanZoomStopped()


So I'd imagine in a complete zooming process, I'll get something like
AsyncPanZoomStarted()
ZoomLevelChanged()
ZoomLevelChanged()
...
AsyncPanZoomStopped()

[1] https://dxr.mozilla.org/mozilla-central/rev/af6356a3e8c56036b74ba097395356d9c6e6c5a3/layout/base/AccessibleCaret.cpp#337-345

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #8)
> So I'd imagine in a complete zooming process, I'll get something like
> AsyncPanZoomStarted()
> ZoomLevelChanged()
> ZoomLevelChanged()
> ...
> AsyncPanZoomStopped()

So yes, it would look something like this

> [1]
> https://dxr.mozilla.org/mozilla-central/rev/
> af6356a3e8c56036b74ba097395356d9c6e6c5a3/layout/base/AccessibleCaret.cpp#337-
> 345

However, you would NOT be able to get an up-to-date zoom using GetCumulativeResolution, as that is only updated at the end of the animation. It is expensive to change the resolution in layout, because it causes a repaint of everything, so we avoid updating that resolution until the user is done pinching. I could send over a zoom level as part of the ZoomLevelChanged notification though, which would be coming from APZ. It would be about as laggy as the AsyncPanZoomStarted/AsyncPanZoomStopped notifications, but it might be more noticeable because the size of the carets would visibly jitter. I think it might be better to hide the carets during zoom instead?

Comment 10

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #9)

On second thought, changing the size of the carets during zooming might cause layout reflow, which might not be good.

> I think it might be better to hide the carets during zoom instead?
Yes. I'm consider the same. At least we could show carets during panning or scrolling, which already improves the current behavior. Could we have an argument to AsyncPanZoomStarted() to distinguish between zooming and panning (scrolling)? Or is there already have a function to query the state of the APZ?

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: WIP to expose zoom begin/end

Unfortunately I don't have a lot of time to spend on this at the moment, but I'm attaching a patch which exposes the relevant information from the APZ side of things. There's some commented-out code where it plumbs into the docshell, feel free to uncomment that and modify it as needed to hook it up however you feel is appropriate. I haven't had a chance to test this but it's fairly straightforward from the APZ side so I'm confident it should work. Hopefully you can pick it up from here - let me know if you have questions or need further assistance with this!

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #5)
> I hack a prototype [1] to prove that my guess in comment #2 is correct, i.e.
> carets can use position:absolute so that APZ can transform them together,
> and carets are continue showing when panning or zooming the page. 
> 
> However, like kats said in comment #1, when the user zooms the carets will
> zoom as well. The size of carets won't be updated to its normal size unless
> the user lifts his fingers off the screen. 
> 
> I cannot tell whether this is already better than the current behavior that
> carets are hidden during panning or zooming, and can be tackled as a
> follow-up. Otherwise this is the best I can do on caret code without other
> modification on APZ.
> 
> Anthony, what do you think? If you have time to test this, the build is in
> [2].

Tested on my N7. I think this is fine for now. Updating this carets as the user zooms is nice but having it resize after isn't necessarily bad either.

:TYLin, after the user lifts his/her fingers off the screen, is it hard to add a 250ms resizing transition? it just seems a bit abrupt right now.

Thanks!

> [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e73b83853755
> [2]
> http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-
> e73b838537554da7a407cb6b8d9b6cffc5075f09/try-android-api-15/fennec-47.0a1.en-
> US.android-arm.apk

Comment 13

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

:kats, thank you for the patch in comment #11. Based on Anthony's comment #12, I feel we might not need to pursue for hiding carets while zooming.

Re comment #12:
> Tested on my N7. I think this is fine for now. Updating this carets as the
> user zooms is nice but having it resize after isn't necessarily bad either.

It's good to hear this feedback. Based on the previous discussion between :kats, updating the carets size as the user zooms never going to have good visual effect.

> :TYLin, after the user lifts his/her fingers off the screen, is it hard to
> add a 250ms resizing transition? it just seems a bit abrupt right now.
It might not be too hard, but I'll need to redesign the carets styling. Will the 250ms transition effect block shipping this bug?

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #13)
> :kats, thank you for the patch in comment #11. Based on Anthony's comment
> #12, I feel we might not need to pursue for hiding carets while zooming.
> 
> Re comment #12:
> > Tested on my N7. I think this is fine for now. Updating this carets as the
> > user zooms is nice but having it resize after isn't necessarily bad either.
> 
> It's good to hear this feedback. Based on the previous discussion between
> :kats, updating the carets size as the user zooms never going to have good
> visual effect.

Can you explain what you mean by this? In Chrome, the carets stay the same "size" as everything else around it is zooming and I think that's a "good visual effect".

For us, we don't keep the carets the same size, but we do it _after_ the user "let's go" (stops zooming) so the abrupt change is what's bad UX behaviour here.

> > :TYLin, after the user lifts his/her fingers off the screen, is it hard to
> > add a 250ms resizing transition? it just seems a bit abrupt right now.
> It might not be too hard, but I'll need to redesign the carets styling. Will
> the 250ms transition effect block shipping this bug?

As above, this lack of a "transition" is quite jarring. Since text-selection is a common interaction, I'd like to get this in if its not too hard. A simple linear transition will work.

Comment 15

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

(In reply to Anthony Lam (:antlam) from comment #14)
> Can you explain what you mean by this? In Chrome, the carets stay the same
> "size" as everything else around it is zooming and I think that's a "good
> visual effect".

Chrome keeps the carets the same size during zooming, which is a good visual effect. However, our implementation is very difficult to do so. If we implement it in an easy way, the visual effect in Firefox will be just bad.

> For us, we don't keep the carets the same size, but we do it _after_ the
> user "let's go" (stops zooming) so the abrupt change is what's bad UX
> behaviour here.

Yes. That's what my prototype does, and I agree the abrupt change is bad.

> As above, this lack of a "transition" is quite jarring. Since text-selection
> is a common interaction, I'd like to get this in if its not too hard. A
> simple linear transition will work.

I'll try to implement this as part of this bug.

Thanks!

Comment 16

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

Anthony,

On second thought, I would like to give the alternative approach :kats mentions in comment #9 a chance for discussion. That is, we continue show the carets while scrolling or panning. But we hide the carets while zooming and show carets again once the user lifts the finger like our current implementation did.

One of my colleagues love this behavior instead, so I'd also like to hear your feedback about this approach.

Comment 17

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #16)
> Anthony,
> 
> On second thought, I would like to give the alternative approach :kats
> mentions in comment #9 a chance for discussion. That is, we continue show
> the carets while scrolling or panning. But we hide the carets while zooming
> and show carets again once the user lifts the finger like our current
> implementation did.
> 
> One of my colleagues love this behavior instead, so I'd also like to hear
> your feedback about this approach.

I understand the cost of re-painting but most users don't really think that way and justify their expectations in that manner.

As a user, when I "create" these text handles, they stay put until I do something to them. This includes moving them, dismissing them, etc. And, when I pinch-to-zoom, I expect things to "zoom" and change size with my gesture. 

Mixing or breaking either of these expectations isn't great UX. So I want to avoid it and provide a consistent experience that the user would likely expect. We could revisit this later too, but for the first update, I'd like to keep the carets around and not have them disappear and reappear, repeatedly. 

hope that helps!

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

:TYLin, just NI-ing in case you missed my comments ^ :)

Comment 19

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

Anthony, thank you for explaining comment #17.

Comment 20

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

(In reply to Xidorn Quan [:xidorn] (UTC+8) from comment #3)
> If using position: absolute is necessary, we can make top layer support
> that. We need this for <dialog> anyway.

Xidorn, have you filed bug or any plan to support a custom content container for anonymous elements in nsCanvasFrame which as position:absolute?

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

It's bug 1236828.

Comment 22

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

Even if bug 1168113 is fixed, as long as the AccessibleCaret divs is under the custom content container, they won't continue showing when scrolling or panning the page with APZ since a container with "-moz-top-layer: top;" will force the carets to be on their own layer.

If we move AccessibleCaret divs out of the custom content container, and let them to be the direct children of the nsCanvasFrame, the caret will continue to show when scrolling or panning the page with APZ, but they won't show on the element in fullscreen mode since the carets will be beneath the fullscreen element.

I would suggest we ship gecko carets on Fennec without fixing this bug.

Comment 23

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

Attachment: MozReview Request: Bug 1249201 Part 2 - Show carets continuously when panning or zooming.

The pref "layout.accessiblecaret.extendedvisibility" is added for Fennec
to keep ActionBar open when carets temporarily hiding during panning or
zooming. Now we make carets always show by default, so the pref can be
removed.

Due to carets are not hiding when OnScrollStart(), many EXPECT_CALL for
|CaretChangedReason::Visibilitychange| are removed from gtest.

Review commit: https://reviewboard.mozilla.org/r/46443/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46443/

Comment 24

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

I might be too pessimistic in comment 22. If bug 1168113 is fixed, with my patch in comment 23, we can fix this bug. The carets might not work on fullscreen element, but I am not worry about the issue for now.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #23)
> Due to carets are not hiding when OnScrollStart(), many EXPECT_CALL for
> |CaretChangedReason::Visibilitychange| are removed from gtest.

This might break the floating text selection. I'm listening to the visibilitychange event in order to hide the toolbar while scrolling the page:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/ActionBarHandler.js#68-70

Is there an other event I could listen to in order to achieve this?

Comment 26

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

Re comment 25: 

From the ActionBarHandler.js, it looks like you unconditionally hide the floating text selection whenever receiving the "visibilitychange" event regardless of the other data in the event. If it's correct, I think we can still dispatch the event in [1] without hiding the carets. Or we could invent a new event to indicate that the carets are scrolling.

[1] https://dxr.mozilla.org/mozilla-central/rev/21bf1af375c1fa8565ae3bb2e89bd1a0809363d4/layout/base/AccessibleCaretManager.cpp#197

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #26)
> From the ActionBarHandler.js, it looks like you unconditionally hide the
> floating text selection whenever receiving the "visibilitychange" event
> regardless of the other data in the event. If it's correct, I think we can
> still dispatch the event in [1] without hiding the carets. Or we could
> invent a new event to indicate that the carets are scrolling.

That's correct. We hide the floating toolbar while scrolling the page. I guess a new event is cleaner assuming that "visibilitychange" refers to the visibility of the carets.

Comment 28

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

Attachment: MozReview Request: Bug 1249201 Part 1 - Add "scroll" reason to CaretStateChangedEvent.

When the carets are scrolled by APZ, they will hide and dispatch a
"visibilitychange" reason. The floating toolbar (ActionBarHandler.js) on
Android listens to the event to update its visibility.

Now we want to show carets continuously when scrolling the page, so it
make no sense to dispatch a "visibilitychange" reason. However we still
need to notify the toolbar that the carets are scrolling by apz.
Therefore, we need a this new "scrollbyapz" reason. It will be dispatch
in AccessibleCaretManager::OnScrollStart() in Part 2.

Review commit: https://reviewboard.mozilla.org/r/47707/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/47707/

Comment 29

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

Comment on attachment 8741379 [details]
MozReview Request: Bug 1249201 Part 2 - Show carets continuously when panning or zooming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46443/diff/1-2/

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8743305 [details]
MozReview Request: Bug 1249201 Part 1 - Add "scroll" reason to CaretStateChangedEvent.

https://reviewboard.mozilla.org/r/47707/#review44577

::: dom/webidl/CaretStateChangedEvent.webidl:14
(Diff revision 1)
>    "updateposition",
>    "longpressonemptycontent",
>    "taponcaret",
>    "presscaret",
> -  "releasecaret"
> +  "releasecaret",
> +  "scrollbyapz"

apz is rather mysterious term for anyone not familiar with browser internals. Why not just "scroll". With that, r+

Comment 31

[Mats Palmgren (inactive)]

Comment on attachment 8741379 [details]
MozReview Request: Bug 1249201 Part 2 - Show carets continuously when panning or zooming.

https://reviewboard.mozilla.org/r/46443/#review44595

Fwiw, I agree with Olli that "scroll" is a better name.

::: layout/base/AccessibleCaretManager.cpp:618
(Diff revision 2)
>  AccessibleCaretManager::OnScrollStart()
>  {
>    AC_LOG("%s", __FUNCTION__);
>  
> +  if (!sCaretsAlwaysShowWhenScrolling) {
> +    // Backup the appearance so that we can restore them after the scrolling is

nit: replace "is ended" with "ends"

Comment 32

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

https://reviewboard.mozilla.org/r/47707/#review44577

> apz is rather mysterious term for anyone not familiar with browser internals. Why not just "scroll". With that, r+

Agree. "scroll" is shorter and cleaner.

Comment 33

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

Comment on attachment 8743305 [details]
MozReview Request: Bug 1249201 Part 1 - Add "scroll" reason to CaretStateChangedEvent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/47707/diff/1-2/

Comment 34

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

Comment on attachment 8741379 [details]
MozReview Request: Bug 1249201 Part 2 - Show carets continuously when panning or zooming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46443/diff/2-3/

Comment 35

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

Comment on attachment 8741379 [details]
MozReview Request: Bug 1249201 Part 2 - Show carets continuously when panning or zooming.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46443/diff/3-4/

Comment 36

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

https://reviewboard.mozilla.org/r/46441/#review44795

Sebastian, need your confirmation that my part 2 won't break floating toolbar.

Comment 37

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

Anthony,

This is the build with the 250ms resizing transition effect per request in comment 12. Please help test whether the effect is good. Thank you.

http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-5afcb10aeca4cd2ef5cc2b94e4805265e2e77fce/try-android-api-15/fennec-48.0a1.en-US.android-arm.apk

Comment 38

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #37)
> Anthony,
> 
> This is the build with the 250ms resizing transition effect per request in
> comment 12. Please help test whether the effect is good. Thank you.
> 
> http://archive.mozilla.org/pub/mobile/try-builds/[e-mail]-
> 5afcb10aeca4cd2ef5cc2b94e4805265e2e77fce/try-android-api-15/fennec-48.0a1.en-
> US.android-arm.apk

Looks good Ting! thanks!

Comment 39

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8741379 [details]
MozReview Request: Bug 1249201 Part 2 - Show carets continuously when panning or zooming.

https://reviewboard.mozilla.org/r/46443/#review44797

Works great!

::: mobile/android/chrome/content/ActionBarHandler.js:68
(Diff revision 4)
>  
>      // Else, update an open ActionBar.
>      if (this._selectionID) {
>        let [element, win] = this._getSelectionTargets();
>        if (this._targetElement === element && this._contentWindow === win) {
> -        if (e.reason == 'visibilitychange' || e.reason == 'presscaret') {
> +        if (e.reason == 'visibilitychange' || e.reason == 'presscaret' ||

I guess we can remove "visibilitychange" here now?

Comment 40

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

https://reviewboard.mozilla.org/r/46443/#review44797

> I guess we can remove "visibilitychange" here now?

There are cases other than "scroll" which hide carets and dispatch "visibilitychange". I guess we still need it to hide the action bar, right?

Comment 41

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

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #40)
> https://reviewboard.mozilla.org/r/46443/#review44797
> 
> > I guess we can remove "visibilitychange" here now?
> 
> There are cases other than "scroll" which hide carets and dispatch
> "visibilitychange". I guess we still need it to hide the action bar, right?

If my assumption is wrong, we should file a follow-up bug to clean that up.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/533b9287d572
https://hg.mozilla.org/integration/mozilla-inbound/rev/989e88261527

Comment 43

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #41)
> (In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #40)
> > https://reviewboard.mozilla.org/r/46443/#review44797
> > 
> > > I guess we can remove "visibilitychange" here now?
> > 
> > There are cases other than "scroll" which hide carets and dispatch
> > "visibilitychange". I guess we still need it to hide the action bar, right?
> 
> If my assumption is wrong, we should file a follow-up bug to clean that up.

Interesting. What are those events? I used "visibilitychange" primarily to hide the toolbar when scrolling the page. I guess this depends on what other actions trigger "visibilitychange".

Comment 44

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

(In reply to Sebastian Kaspari (:sebastian) from comment #43)
> Interesting. What are those events? I used "visibilitychange" primarily to
> hide the toolbar when scrolling the page. I guess this depends on what other
> actions trigger "visibilitychange".

The most common case is the blur event. We need to hide the carets [1] as well as the actionbar. Other cases like range collapsing to start or end, etc., are also calling HideCarets() and dispatch "visibilitychange" in [2].

However, I just realize that carets are hidden in these cases, so it should be handled in [3] in ActionBarHandler.js. If the sole purpose of the old "visibilitychange" is for hiding the actionbar during scrolling the page, we can remove it.

[1] https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/layout/base/AccessibleCaretManager.cpp#689-693
[2] https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/layout/base/AccessibleCaretManager.cpp#137
[3] https://dxr.mozilla.org/mozilla-central/rev/0891f0fa044cba28024849803e170ed7700e01e0/mobile/android/chrome/content/ActionBarHandler.js#33

Comment 45

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #44)
> However, I just realize that carets are hidden in these cases, so it should
> be handled in [3] in ActionBarHandler.js. If the sole purpose of the old
> "visibilitychange" is for hiding the actionbar during scrolling the page, we
> can remove it.

Ok. We can do some testing in a follow-up bug and then maybe remove it.. :)

Comment 46

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/533b9287d572
https://hg.mozilla.org/mozilla-central/rev/989e88261527