<a class="header-button" href="https://bugzilla.mozilla.org/home" title="Go to home page"> Bugzilla

Assignee

Comment 1

•

9 years ago

If the carets are dismiss due to blur event but the selection highlight is still there, scrolling the content a bit will bring the carets back by [1]. I'll see what I can do. [1] https://dxr.mozilla.org/mozilla-central/rev/f0f1aaf051d6798e1e73d1feee07ca847333167a/layout/base/AccessibleCaretManager.cpp#703

Assignee: nobody → tlin

Blocks: AccessibleCaret

Status: NEW → ASSIGNED

Flags: needinfo?(tlin)

Comment hidden (mozreview-request)

Comment 4

•

9 years ago

I don't quite follow why this bug occurs. Do we get an OnScrollEnd on the old document or the new document? If it's the new document (which I assume), why do we even have active selection carets there if no selection has occurred in the new tab?

Flags: needinfo?(tlin)

Comment 5

•

9 years ago

Comment on attachment 8803282 [details] Bug 1310509 - Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content. Cancelling the review until the questions in comment 4 are addressed.

Attachment #8803282 - Flags: review?(mats)

Assignee

Comment 6

•

9 years ago

(In reply to Mats Palmgren (:mats) from comment #4) > I don't quite follow why this bug occurs. Do we get an OnScrollEnd on the old document > or the new document? If it's the new document (which I assume), why do we even have > active selection carets there if no selection has occurred in the new tab? In adb logcat, I find that we get OnScollEnd() called on every PresShell. In this bug, it's old document in the background tab which still have a selection that fires the CaretStateChanged event which makes the floating text selection menu shown. The new document doesn't have any selection or carets. I don't know the details that the selection is going to be cleared or not when switching tabs. The selection is often cleared when I click the number on the top right corner to switch tab. However if I switch tab via the prompt "New tab opened SWITCH", it doesn't clear the selection. The solution I propose could also fix bug 1171843 which has the similar scenario but it's in the same document. That's why I think we should fix the AccessibleCaret by not bring the carets back when they're hidden by previous blur event. (And sorry for the delayed response.)

Flags: needinfo?(tlin) → needinfo?(mats)

Comment 7

•

9 years ago

OK, so switching tab blurs the document in the current tab, and that blur event triggers OnScrollEnd(?) which hides the carets? And the tab you switch to gets a focus event on its document which is supposed to restore the carets? Perhaps the problem is that these events does not occur in the expected order? Or, are not fired at all? If so, we should probably look into that first, since it seems quite important in general that we get these events in a reliable order. Anyway, personally I do think that we should restore the carets when switching back to a tab that had a selection. Switching between tabs shouldn't be "destructive" to the selection state IMO.

Flags: needinfo?(mats)

Comment 8

•

9 years ago

BTW, it seems layout.accessiblecaret.enabled=true isn't working to enable the carets in my local Linux debug build anymore. Is there some other pref I need to set as well?

Assignee

Comment 9

•

9 years ago

Re comment 7: > OK, so switching tab blurs the document in the current tab, and that blur event > triggers OnScrollEnd(?) which hides the carets? The blur event hides the carets in the old tab (step 4 in comment 0), and the subsequent scroll-down (step 5) on the new tab triggers the OnScrollEnd() in the old tab, which dispatches the CaretStateChanged event that makes the floating toolbar shown. > And the tab you switch to gets a focus event on its document which is supposed > to restore the carets? The new tab doesn't have a new selection unless the user long taps on something. > Perhaps the problem is that these events does not occur in the expected order? > Or, are not fired at all? I think the event order is not the problem. The problem is if a selection doesn't have carets accompanied due to the previous blur, do we want to restore carets via scrolling in OnScrollEnd()? > Anyway, personally I do think that we should restore the carets when switching > back to a tab that had a selection. Switching between tabs shouldn't be > "destructive" to the selection state IMO. I totally agree! Ideally, if the selection is *visible* to the user (not in the background tab or covered by other dom element like the Wikipedia in bug 1171843), we should update the carets. However, I had no idea how to determine whether the selection is visible or not. It's also possible that only part of the selection highlight is covered. Things get tricky. It's doable to hide the carets on blur event, and not bring them back in any circumstances except the user long tap on an existing selection or on a new word. Re comment 8: > BTW, it seems layout.accessiblecaret.enabled=true isn't working to enable > the carets in my local Linux debug build anymore. Is there some other > pref I need to set as well? Oh. You'll need to turn off "layout.accessiblecaret.hide_carets_for_mouse_input". On Linux desktop, you might want to try the STR in bug 1171843 comment 0. And "layout.accessiblecaret.use_long_tap_injector" was default to false now, you'll need to use double-click or drag to make a new selection.

Flags: needinfo?(mats)

Comment 10

•

9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #9) > The blur event hides the carets in the old tab (step 4 in comment 0), OK, good. > the subsequent scroll-down (step 5) on the new tab triggers the > OnScrollEnd() in the old tab This seems wrong to me. Scrolling in the new tab shouldn't trigger events in the old tab, those events should go to the new tab. Since the problem here seems related to event handling, perhaps enndeakin / smaug can give better advice?

Flags: needinfo?(mats)

Flags: needinfo?(enndeakin)

Flags: needinfo?(bugs)

Olli Pettay [:smaug][bugs@pettay.fi]

Assignee

Comment 11

•

9 years ago

The OnScrollEnd() is called by AccessibleCaretEventHub::AsyncPanZoomStopped() [1], which is registered via nsIScrollObserver. I'm not sure whether APZ should distinguish the observer is in the background or not. [1] http://searchfox.org/mozilla-central/rev/4012e61cc38758ffa1f7ce26039173f67b048853/layout/base/AccessibleCaretEventHub.cpp#727

Comment 12

•

9 years ago

That doesn't sounds really event handling related. Shouldn't the observer unregister itself when the docshell goes to background?

Flags: needinfo?(bugs)

Assignee

Comment 13

•

9 years ago

(In reply to Olli Pettay [:smaug] from comment #12) > That doesn't sounds really event handling related. > Shouldn't the observer unregister itself when the docshell goes to background? It seems difficult (or probably overkill) to register/unregister the listener every time the visibility of a document is changed. An easy solution would be just ignore the APZ callbacks when the document is not visible.

Flags: needinfo?(enndeakin)

Comment hidden (mozreview-request)

Comment 15

•

9 years ago

mozreview-review

Comment on attachment 8803282 [details] Bug 1310509 - Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content. https://reviewboard.mozilla.org/r/87588/#review89300 Why is this a problem only for AsyncPanZoomStarted/Stopped and not ScrollPositionChanged etc? IIRC, we have one of these listeners per presshell? So it seems it would be a performance problem to have all of those listening for events. Maybe not so much on mobile, where I assume the number of tabs is low, but on desktop it's not uncommon to have hundreds of shells, so if we want to enable these carets on desktop we should probably address this potential performance issue first. Perhaps we could add something to PresShell::Freeze()/Thaw() to (de)register the listeners? This is probably more enndeakin / smaug's area of expertise than mine, so I'd suggest you get review from one of them instead. Thanks.

Attachment #8803282 - Flags: review?(mats)

Assignee

Comment 16

•

9 years ago

Re comment 15: > Why is this a problem only for AsyncPanZoomStarted/Stopped and not ScrollPositionChanged etc? My bad. ScrollPositionChanged should be included as well. > IIRC, we have one of these listeners per presshell? So it seems it would be a performance > problem to have all of those listening for events. Maybe not so much on mobile, where > I assume the number of tabs is low, but on desktop it's not uncommon to have hundreds > of shells, so if we want to enable these carets on desktop we should probably address > this potential performance issue first. Yes. We have one AccessibleCaretEventHub per PresShell. I've done some test locally, those listeners only trigger on current tab on my Linux desktop, so I think there's no performance issue on desktop. > Perhaps we could add something to PresShell::Freeze()/Thaw() to (de)register the listeners? On Fennec, switching tabs do not trigger PresShell::Freeze()/Thaw(). > This is probably more enndeakin / smaug's area of expertise than mine, so I'd suggest > you get review from one of them instead. Thanks. Mats, thank you for the help so far :)

Assignee

Comment 17

•

9 years ago

> I've done some test locally, those listeners only trigger on current tab on my Linux desktop, so I think there's no performance issue on desktop. This is untrue for desktop with non-e10s. The behavior for non-e10s is like Fennec. I'm not sure I understand the relationship between APZ and the document structure. Here's what I have observed. On Fennec, after the scroll ended, APZEventState::ProcessAPZStateChange() calls NotifyAsyncPanZoomStopped() with the top level document (I assume it's the chrome UI) [1]. And then the function calls docShell->NotifyAsyncPanZoomStopped() [2] for all it's child docshells which are all the tabs. That's why all the observers of AccessibleCaretEventHub in all PresShell get called. I see two possible solutions for this. 1) Call NotifyAsyncPanZoomStarted/Stopped etc only if the document is not hidden assuming the listeners in background tab won't be interested in the callbacks. Not sure if this makes sense. 2) Otherwise, I'll go early return if the document is hidden in callbacks AccessibleCaretEventHub, and update patch with Mats' comment 15 addressed. Kats, this is somewhat related to APZ, so beside smaug, perhaps you'll have some insight. Thanks! [1] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/gfx/layers/apz/util/APZEventState.cpp#450-454 [2] http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/docshell/base/nsDocShell.cpp#3113

Flags: needinfo?(bugs)

Flags: needinfo?(bugmail)

Assignee

Comment 18

•

9 years ago

Third solution is to implement nsIDocumentActivity for AccessibleCaretEventHub, and register/unregister itself when document goes to foreground/backgound as smaug suggested in comment 12. I'll try it this works as expected.

Flags: needinfo?(bugs)

Flags: needinfo?(bugmail)

Comment hidden (mozreview-request)

Comment 20

•

9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #17) > 1) Call NotifyAsyncPanZoomStarted/Stopped etc only if the document is not > hidden assuming the listeners in background tab won't be interested in the > callbacks. Not sure if this makes sense. Something we could do is just fire the NotifyAsyncPanZoomStarted/Stopped on the innermost document that contains the scrolling thing. That might cut down on the perf hit and also fix this bug. If you want to try it the code in APZEventState::ProcessAPZStateChange would need to be modified to get the document from the viewId rather than using the aDocument that is passed in.

Olli Pettay [:smaug][bugs@pettay.fi]

Comment 21

•

9 years ago

Oh I guess we might have to move the mActiveAPZTransforms counter into the document as well.

Comment 22

•

9 years ago

mozreview-review

Comment on attachment 8803282 [details] Bug 1310509 - Call NotifyAsyncPanZoomStarted/Stopped on document containing current scrolling content. https://reviewboard.mozilla.org/r/87588/#review89744 Looks ok to me, but sounds like kats had some different idea too. I guess both approaches work.

Attachment #8803282 - Flags: review?(bugs) → review+

Assignee

Comment 23

•

9 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20) I'm happy with my latest patch, but I would like to learn a bit more about your approach before I land the patch because modifying something in APZ takes more time for me :) > Something we could do is just fire the NotifyAsyncPanZoomStarted/Stopped on > the innermost document that contains the scrolling thing. If the innermost document is an iframe, do you mean that we just fire NotifyAsyncPanZoomStarted/Stopped on it? What if I'm scrolling the parent document of the iframe? > That might cut down on the perf hit and also fix this bug. If you want to try it the code > in APZEventState::ProcessAPZStateChange would need to be modified to get the > document from the viewId rather than using the aDocument that is passed in. My APZ-fu is a bit weak... How to get the document from the viewID?

Comment 24

•

9 years ago

(In reply to Ting-Yu Lin [:TYLin] (UTC+8) from comment #23) > If the innermost document is an iframe, do you mean that we just fire > NotifyAsyncPanZoomStarted/Stopped on it? What if I'm scrolling the parent > document of the iframe? If you're scrolling the parent document then the "innermost document that contains the scrolling thing" would be the parent document, rather than the iframe, so we'd fire the Notify methods on the parent rather than the iframe. > My APZ-fu is a bit weak... How to get the document from the viewID? The ViewID maps to an nsIContent, which you can then use to get the document: nsIContent* content = nsLayoutUtils::FindContentFor(viewId); nsIDocument* doc = content ? content->GetComposedDoc() : nullptr;

Comment hidden (mozreview-request)