Closed Bug 917297 Opened 8 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

All
Android
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 27

People

(Reporter: kats, Assigned: capella)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch bug917297 (v1) (obsolete) — Splinter Review
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?
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.
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
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.
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: nobody → markcapella
Status: NEW → ASSIGNED
Attached patch bug917297 (v2)Splinter Review
This tests out nice ... I think our work here is done :-P
Attachment #807065 - Attachment is obsolete: true
Attachment #808209 - Flags: review?(bugmail.mozilla)
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+
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.