917297 - Clicking on a link sometimes results in a JS error to logcat instead of navigation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

From bug 901962:

(In reply to Pop Mihai from comment #31)
> So after a few tries, I was able to reproduce the behaviour described by
> [e-mail]; 
> I've opened
> "https://bug901962.bugzilla.mozilla.org/attachment.cgi?id=786396" 
> 1. Zoom out.
> 2. Tap on link "Seven Ways..."
> 3. Tap back.
> 4. Zoom out.
> 5. Tap on link "Seven Ways..."
> 
> Actual result:
> You can see visual feedback, but there is not action to open the link.
> 
> I get this error in the log console whenever I tap on the link and the link
> does not open:
> 
> 09-16 15:20:59.363: E/GeckoConsole(11831): [JavaScript Error: "aElement is
> null" {file: "chrome://browser/content/browser.js" line: 4584}]

I can reproduce this as well.

Comment 1

[Mark Capella [:capella]]

Attachment: bug917297 (v1)

It looks like the code in "Gesture:SingleTap" hits a situation where it knows the element isElementClickable(), but then tries to _moveClickPoint() (???) and subsequently believes the user actually missed clicking the clickable element ...

The attached patch handles the element == null gracefully, but I don't understand the basic train of thought there ...

Comment 2

[Wesley Johnston (:wesj)]

Comment on attachment 807065 [details] [diff] [review]
bug917297 (v1)

I have a feeling the problem here is in moveClickPoint hitting the edge of an element. But anyElementFromPoint should always return an element to us.

Comment 3

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

(In reply to Mark Capella [:capella] from comment #1)
> but I don't
> understand the basic train of thought there ...

In a nutshell the element-finding tap logic goes like this:
(1) The first event browser.js gets is a touchstart, which runs the _handleTouchStart function. The second half of this function does a fuzzy hit test to see if there are any clickable elements within the touch radius, and sets that to _highlightElement in the doTapHighlight function. If there are no clickable elements in the area, it will take the element directly under that touch point and use that (Therefore, _highlightElement may or may not be clickable; if it is clickable it may not be directly under the touch point)
(2) When the user's finger goes back up and we get the Gesture:SingleTap event in browser.js, we want to send a click event to that element if it is clickable. The check for isElementClickable and _moveClickPoint is supposed to handle the case where the element is clickable but not directly under the touch point. When we create a click event and dispatch it to Gecko, we send touch coordinates rather than the element we want to dispatch it on, so we need to adjust the touch point to be directly over the clickable element. That is what _moveClickPoint does.
(3) If the element is not clickable, then the _moveClickPoint doesn't get run, and we just send the click event to the exact coordinates of the touch point.

So if _moveClickPoint is getting called, but then anyElementFromPoint is returning null, then we need to adjust the code in _moveClickPoint so that it moves the coordinates differently, so that anyElementFromPoint returns the element it's supposed to.

Does that make sense?

Comment 4

[Mark Capella [:capella]]

Cool! Thanks kats! This is a new area I'm exploring, so I'm kinda debugging my way through it,

In the failure case, _moveClickPoint() is finding two client rects for the element ... the clickpoint is outside the (0) element but within the (1) element, so the routine returns the same x/y that was passed to it.

anyElementFromPoint() is called and the very first request to cwu.elementFromPoint() returns null, after calling -> nsDOMWindowUtils::ElementFromPoint() -> nsDocument::ElementFromPointHelper() -> nsLayoutUtils::GetFrameForPoint() which finds |outFrames.Length() == 0| and then we bail backward from here: http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#3099

Comment 5

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

Ah, so in that mxr link you pasted, is aIgnoreRootScrollFrame true or false? I think it should be using true, but I suspect it might be false, which would explain this behaviour.

Comment 6

[Mark Capella [:capella]]

Ooooh ... I actually wondered about that ... we call setting it to false
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#4669

Comment 7

[Mark Capella [:capella]]

And in source/dom/browser-element/BrowserElementPanning.js there's an almost identical routine that uses |true|
http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js#822

Comment 8

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

(In reply to Mark Capella [:capella] from comment #6)
> Ooooh ... I actually wondered about that ... we call setting it to false
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#4669

Yeah that should be passing in true, I think. Same for the one a few lines down.

Comment 9

[Mark Capella [:capella]]

Right ... I'm going to go ahead and test it before / after ... 

Assuming it corrects this case, would you know if the fix is best:
   1) change the anyElementFromPoint() method which has three consumers (including the one we're trying to fix)? Or 
   2) add a parm to the method to just change the behaviour for this particular case?

Comment 10

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

I think we want to just change anyElementFromPoint because I think we always want that behaviour.

Comment 11

[Mark Capella [:capella]]

Attachment: bug917297 (v2)

This tests out nice ... I think our work here is done :-P

Comment 12

[Mark Capella [:capella]]

https://tbpl.mozilla.org/?tree=Try&rev=260c06810467

Comment 13

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

Comment on attachment 808209 [details] [diff] [review]
bug917297 (v2)

Review of attachment 808209 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM. Thanks for digging into this!

Comment 14

[Mark Capella [:capella]]

Bonus points whenever I learn tweaky new stuff  ;-)
https://hg.mozilla.org/integration/fx-team/rev/9db9adaff764

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9db9adaff764