Continue showing AccessibleCaret when scrolling or panning the page

RESOLVED FIXED in Firefox 48

Status

()

Core
Selection
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: TYLin, Assigned: TYLin)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

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.
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.
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
Flags: needinfo?(bugmail.mozilla)
If using position: absolute is necessary, we can make top layer support that. We need this for <dialog> anyway.
Summary: Continue showing AccessibleCaret when scrolling for panning the page → Continue showing AccessibleCaret when scrolling or panning the page
(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.
Flags: needinfo?(bugmail.mozilla)
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/tlin@mozilla.com-e73b838537554da7a407cb6b8d9b6cffc5075f09/try-android-api-15/fennec-47.0a1.en-US.android-arm.apk
Flags: needinfo?(alam)
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.
Flags: needinfo?(bugmail.mozilla)
(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.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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
Flags: needinfo?(bugmail.mozilla)
(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?
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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?
Flags: needinfo?(bugmail.mozilla)
Created attachment 8722537 [details] [diff] [review]
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!
Flags: needinfo?(bugmail.mozilla)
(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/tlin@mozilla.com-
> e73b838537554da7a407cb6b8d9b6cffc5075f09/try-android-api-15/fennec-47.0a1.en-
> US.android-arm.apk
Flags: needinfo?(alam) → needinfo?(tlin)
: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?
Flags: needinfo?(alam)
Assignee: nobody → tlin
Status: NEW → ASSIGNED
Flags: needinfo?(tlin)
(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.
Flags: needinfo?(alam) → needinfo?(tlin)
(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!
Flags: needinfo?(tlin)
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.
Flags: needinfo?(alam)
(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!
Flags: needinfo?(alam)

Updated

2 years ago
Blocks: 1097398
:TYLin, just NI-ing in case you missed my comments ^ :)
Flags: needinfo?(tlin)
Anthony, thank you for explaining comment #17.
Flags: needinfo?(tlin)
(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?
Flags: needinfo?(quanxunzhen)
It's bug 1236828.
Flags: needinfo?(quanxunzhen)

Updated

2 years ago
Blocks: 1168847
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.
Assignee: tlin → nobody
Status: ASSIGNED → NEW
Created attachment 8741379 [details]
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/
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.

Updated

2 years ago
Blocks: 1230609
No longer blocks: 1168847
(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?
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
(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.
Created attachment 8743305 [details]
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/
Attachment #8741379 - Attachment description: MozReview Request: Bug 1249201 - Show carets continuously when panning or zooming. → MozReview Request: Bug 1249201 Part 2 - Show carets continuously when panning or zooming.
Attachment #8743305 - Flags: review?(bugs)
Attachment #8741379 - Flags: review?(mats)
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 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+
Attachment #8743305 - Flags: review?(bugs) → review+
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"
Attachment #8741379 - Flags: review?(mats) → review+
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 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/
Attachment #8743305 - Attachment description: MozReview Request: Bug 1249201 Part 1 - Add "scrollbyapz" reason to CaretStateChangedEvent. → MozReview Request: Bug 1249201 Part 1 - Add "scroll" reason to CaretStateChangedEvent.
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 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/
Assignee: nobody → tlin
Status: NEW → ASSIGNED
https://reviewboard.mozilla.org/r/46441/#review44795

Sebastian, need your confirmation that my part 2 won't break floating toolbar.
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/tlin@mozilla.com-5afcb10aeca4cd2ef5cc2b94e4805265e2e77fce/try-android-api-15/fennec-48.0a1.en-US.android-arm.apk
Flags: needinfo?(alam)
(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/tlin@mozilla.com-
> 5afcb10aeca4cd2ef5cc2b94e4805265e2e77fce/try-android-api-15/fennec-48.0a1.en-
> US.android-arm.apk

Looks good Ting! thanks!
Flags: needinfo?(alam)
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?
Attachment #8741379 - Flags: review?(s.kaspari) → review+
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?
(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.
(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".
(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
(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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/533b9287d572
https://hg.mozilla.org/mozilla-central/rev/989e88261527
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.