If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED in Firefox 27

Status

()

Firefox for Android
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: capella)

Tracking

unspecified
Firefox 27
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
> costa@innovationbound.com; 
> 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.
(Assignee)

Comment 1

4 years ago
Created attachment 807065 [details] [diff] [review]
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 ...
Attachment #807065 - Flags: feedback?(wjohnston)
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.
Attachment #807065 - Flags: feedback?(wjohnston) → feedback-
(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?
(Assignee)

Comment 4

4 years ago
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
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.
(Assignee)

Comment 6

4 years ago
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
(Assignee)

Comment 7

4 years ago
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
(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.
(Assignee)

Comment 9

4 years ago
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?
I think we want to just change anyElementFromPoint because I think we always want that behaviour.
(Assignee)

Updated

4 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 11

4 years ago
Created attachment 808209 [details] [diff] [review]
bug917297 (v2)

This tests out nice ... I think our work here is done :-P
Attachment #807065 - Attachment is obsolete: true
Attachment #808209 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 12

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=260c06810467
Comment on attachment 808209 [details] [diff] [review]
bug917297 (v2)

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

LGTM. Thanks for digging into this!
Attachment #808209 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 14

4 years ago
Bonus points whenever I learn tweaky new stuff  ;-)
https://hg.mozilla.org/integration/fx-team/rev/9db9adaff764
https://hg.mozilla.org/mozilla-central/rev/9db9adaff764
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Blocks: 920862
You need to log in before you can comment on or make changes to this bug.