1171731 - Zoomed View unexpectedly activates when textSize=0

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

* Go to http://www.wsj.com/articles/in-hong-kong-the-apartments-are-fit-for-a-mosquito-1433237582?mod=e2fb
* Scroll down to the image carousel
* Hit one of the cycle buttons

Expected: The image changes
Actual: The zoomed view appears

The hit target seems large enough that I wouldn't expect this to happen - any ideas, Dominique?

By the way, you can use remote debugging on your Android device to try to figure out what's going on in the site's mobile HTML.

Comment 1

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

Attachment: Screenshot of the issue

Comment 2

[Dominique Vincent [:domivinc]]

Hi Michael, the html of the button is the following:

<button type="button" data-role="none" class="slick-next" aria-label="next" style="display: block;">Next</button>

To avoid to display the text "Next" with the background picture of the button, the font size has been set to 0: "font-size: 0;" in the class "slick-next".

The cluster detection process detects that the button text is too small to be readable (0 size), so the zoomed view shows up.

One possible fix is to consider that all the elements using a 0 font size should be ignored in the cluster detection process.

Comment 3

[Dominique Vincent [:domivinc]]

Attachment: patch-05062015 2-Bug_1171731___Ignore_elements_with_0_font_size_in_cluster_detection__r_kats.patch

Michael, I added the test for elements with font size set to 0.
It seems to fix the issue on my device.

Kats should probably review this change in PositionedEventTargeting.cpp ?

Comment 4

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

Dominique, the patch worked for me. :)

Comment 5

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

Comment on attachment 8615995 [details] [diff] [review]
patch-05062015 2-Bug_1171731___Ignore_elements_with_0_font_size_in_cluster_detection__r_kats.patch

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

::: layout/base/PositionedEventTargeting.cpp
@@ +416,5 @@
>    nsRefPtr<nsFontMetrics> fm;
>    nsLayoutUtils::GetFontMetricsForFrame(aFrame, getter_AddRefs(fm),
>      nsLayoutUtils::FontSizeInflationFor(aFrame));
>    if (fm) {
> +    if ((fm->EmHeight() > 0) && // See bug 1171731

This is fine, but you can also combine it with the enclosing if statement:

if (fm && fm->EmHeight() > 0 &&
    pc->AppUnitsToGfxUnits.....) {
  return false;
}

Comment 6

[Dominique Vincent [:domivinc]]

https://tbpl.mozilla.org/?tree=Try&rev=6eb7893ec049

Comment 7

[Pulsebot]

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

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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