Closed Bug 1176453 Opened 5 years ago Closed 4 years ago

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

Categories

(Firefox for Android :: General, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed
fennec Nightly+ ---

People

(Reporter: blassey, Assigned: domivinc)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
(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.
Assignee: nobody → domivinc
Blocks: zoomedview
Flags: needinfo?(domivinc)
Summary: Magnifying glass is mostly confusing → Check box state changes when zoomed view appears on ambiguous check box click
Attached image 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
Flags: needinfo?(domivinc)
Kats, could you review this patch?
It fixes the issue regarding the label/input elements wrongly considered to be a cluster.
Attachment #8628432 - Flags: review?(bugmail.mozilla)
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/
Attachment #8628432 - Flags: review?(bugmail.mozilla) → review+
tracking-fennec: ? → Nightly+
All changes done! Thanks for the code review Kats.
Attachment #8628432 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e96a4935c5de
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.