1176453 - Check box state changes when zoomed view appears on ambiguous check box click

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Brad Lassey [:blassey] (use needinfo?)]

For example, I just tried to log into the economist. I clicked on the check box to remember my login. It successfully checked off, but also popped op the magnifying glass which obscured the entire form, but did not actually sure the check box.

If we decide to show the magnifying glass, we should prevent the couch from doing anything, since presumably the couch was ambiguous. Also, in a situation stuck as this, the couch is not ambiguous at all (there is not another clickable element anywhere close to this check box), we airtight show the magnifying glass. Perhaps this should be two separate bugs.

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #0)
> If we decide to show the magnifying glass, we should prevent the couch from
> doing anything, since presumably the couch was ambiguous.

FTR, I think "couch" -> "touch". :)

Dominique, do you mind checking this out?

> Also, in a
> situation stuck as this, the couch is not ambiguous at all (there is not
> another clickable element anywhere close to this check box), we airtight
> show the magnifying glass. Perhaps this should be two separate bugs.

bug 1135369.

Comment 2

[Dominique Vincent [:domivinc]]

Attachment: theeconomist.png

I found 2 issues:

1) the touch position is not correctly set in current nightly code on my device (HTC, Android version 2.3). For instance in the attached view, you can touch the white area on top of “Don’t have an account?” and it will trigger the button “Log in”. It’s already a strange effect detected in bug 1175536 (comment 6 [1]). The cluster areas are wrongly detected due to this bug. The bug is no more visible if you scroll down in the page (in the economist login page, you can scroll when the keyboard is displayed).

2) I found a bug in method GetClosest [2], a cluster is detected when a touch occurs near the check box element and the label element of the check box. I will fix it.

I cannot reproduce the following issue: the check box is updated and the zoomed view is also displayed at the same time on the same touch. One possibility to get such behavior could be to make a first touch on the check box and then a second touch near the check box. I have no other idea to reproduce it. It could also be a side effect of the 2 other bugs.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1175536#c6
[2] https://dxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp#324

Comment 3

[Dominique Vincent [:domivinc]]

Attachment: patch-01072015 1-Bug_1176453___Do_not_increment_the_cluster_counter_for_input_elements_with_label__r_kats.patch

Kats, could you review this patch?
It fixes the issue regarding the label/input elements wrongly considered to be a cluster.

Comment 4

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

Comment on attachment 8628432 [details] [diff] [review]
patch-01072015 1-Bug_1176453___Do_not_increment_the_cluster_counter_for_input_elements_with_label__r_kats.patch

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

Looks good, just some minor changes below. r+ with those fixed. Thanks!

::: layout/base/PositionedEventTargeting.cpp
@@ +172,5 @@
>           elm->HasListenersFor(nsGkAtoms::ontouchend);
>  }
>  
>  static bool
> +IsElementClickable(nsIFrame* aFrame, nsIAtom* stopAt = nullptr, nsAutoString* labelForId = nullptr)

I'd prefer calling this variable "aLabelTargetId". "labelForId" seems more ambiguous.

@@ +326,5 @@
>  }
>  
> +// Search in the list of frames aCandidates if the element with the id "labelForId"
> +// is present.
> +static bool isElementPresent(nsTArray<nsIFrame*>& aCandidates, const nsAutoString& labelForId)

Capitalize first letter in method name. Also s/labelForId/aLabelTargetId/

@@ +351,5 @@
>    // Lower is better; distance is in appunits
>    float bestDistance = 1e6f;
>    nsRegion exposedRegion(aTargetRect);
>    for (uint32_t i = 0; i < aCandidates.Length(); ++i) {
> +    nsAutoString labelForId;

Move this down to just before the IsElementClickable call. No need to have it so far away. Also rename to labelTargetId.

@@ +388,5 @@
>        continue;
>      }
>  
> +    // If the first clickable ancestor of f is a label element
> +    // and "for" attribut is present in label element, search the frame list for the "for" element

s/attribut/attribute/

Comment 5

[Dominique Vincent [:domivinc]]

Attachment: patch-02072015 1-Bug_1176453___Do_not_increment_the_cluster_counter_for_input_elements_with_label__r_kats.patch

All changes done! Thanks for the code review Kats.

Comment 6

[Dominique Vincent [:domivinc]]

https://tbpl.mozilla.org/?tree=Try&rev=92a1a0e99f05

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e96a4935c5de

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e96a4935c5de