Closed Bug 1699147 Opened 3 years ago Closed 3 years ago

[Double-tap zoom] The zoom in level is too high for some page (table) elements

Categories

(Core :: Panning and Zooming, defect, P2)

Firefox 88
All
macOS
defect

Tracking

()

RESOLVED FIXED
90 Branch
Tracking Status
firefox88 --- wontfix
firefox89 --- fixed
firefox90 --- fixed

People

(Reporter: emilghitta, Assigned: tnikkel)

References

Details

(Whiteboard: [proton-uplift])

Attachments

(3 files, 1 obsolete file)

Affected versions

  • Firefox 88.0a1 (BuildId:20210317095331)

Affected platforms

  • macOS 10.14
  • macOS 11.1

Unaffected platforms

  • Windows 10 64bit
  • Ubuntu 20.04

Preconditions

  • Have apz.mac.enable_double_tap_zoom_touchpad_gesture pref enabled

Steps to reproduce

  1. Launch Firefox.
  2. Access the following link.
  3. Double-tap to zoom inside the right table (ex: over the Government section).

Expected result

  • As in Chrome or Safari, the right amount of "zoom in" level is applied which doesn’t lead to portions of text being displayed as cut out of the viewport.

Actual result

  • The "zoom in" level is too high which can lead to some situations where portions of text are displayed as cut out of the viewport and the user has to manually adjust the desired focus area.

Regression Range

  • I don’t think that this is a regression.

Notes

  • For further information regarding this issue, please observe the following screencast (Unfortunately the file size exceeds the bugzilla limit. Mozilla account needed).
  • I've noticed that this issue mostly occurs on table elements.

I think that this is not related to the content itself, but the fact that Chrome and Safari have a lower maximum zoom level than us. All browsers in the video zoom to the max level that is allowed to fit the content by width.

I think the general logic we use is fine, so we might want to consider reducing the max zoom level if it causes problems.

Has STR: --- → yes

:tnikkel, thinking about this a bit, I think it would be good if we could match Safari's max-level here. I feel that we zoom in a bit too far in some cases. WDYT? Would this affect pinch-zooming as well?

Flags: needinfo?(tnikkel)
Priority: -- → P2

I haven't looked at this too much yet, so I don't have much of an opinion. Matching Safari/Chrome's max level seems reasonable, although personally I do like our max level being pretty high for pinch zoom, so hopefully they can be specified separately, I'll have to look in more detail.

Flags: needinfo?(tnikkel)

Agreed. A high level for pinch-zooming is nice, and a lot easier to control as a user. Treating double-tap and pinch max levels separate would be ideal.

This patch limits the change to desktop, mobile might not want this.

It only affects zooming in, not zooming out.

This means that it's possible to have the following sequence: double tap zoom in, double tap zoom in more, double tap zoom all the way out.

This feels kind of natural to me, but I'm not a UX person.

Assignee: nobody → tnikkel
Status: NEW → ASSIGNED
Attached video double-tap.mp4

I tested the patch and I feel like it emphasizes bug 1699451 even further. For example, in the attached video I go to https://www.wikipedia.org/ and double-tap on a language. Then I double-tap again to reset the zoom, but instead I get moved to another element. Also, I tried to double-tap the "globe" to reset the zoom, but got focused in just a tiny bit more.

Comparing the same steps in Safari, the first focus is already different. It moves the language I double-tap on closer to the center of the viewport. Another double-tap resets.

I'm not sure if this confusion comes from the second zoom step, or if it's related to focusing the wrong element.

I just compared the same steps to the current Nightly, and there it resets correct after the second double-tap on the language, which I feel is a more predictable experience.

Thanks for testing. It could definitely make bug 1699451 worse. To properly evaluate this we'll need to get that but fixed, fortunately I'm working on that.

Another possible solution would be the following (on desktop only): double tap zoom can increase the zoom level to 3x at most, any double tap when the zoom level is 3x or higher (which can be arrived at by pinching) would reset zoom to 1x.

Any other ideas? We can also not do anything and accept this behaviour.

I've been thinking about this a bit, and I think your suggestion to always reset when at max level could make sense. Though I think I'd like to try this patch again once bug 1699451 has a solution before we try to optimize this one :)

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:tnikkel, could you have a look please?
For more information, please visit auto_nag documentation.

Flags: needinfo?(tnikkel)
Flags: needinfo?(aklotz)
Flags: needinfo?(aklotz)

We're not sure if we want to land that specific patch.

Flags: needinfo?(tnikkel)

Notes from discussion with Martin:

  • This is not a release blocker but nice-to-have if we can fix it in time.
  • Taking another look with bug 1698537 fixed will help us figure out the approach here.
Depends on: 1698537

Another idea: add a heuristic specifically for table cells. Something like if the width of the table cell is less than 30% of the viewport width use the parent element of the table cell to compute the double tap target rect. This means we won't zoom to a single table cell in tables that are being used as a traditional "data" table, while still letting us zoom to table cells that are used for layout like it's 2005.

We have to look at the element or its parent to fix the wikipedia case because there is a display block div inside the table cell.

Summary: [Double-tap zoom] The zoom in level is to high for some page (table) elements → [Double-tap zoom] The zoom in level is too high for some page (table) elements
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63d854871f8b
Don't choose table cells smaller than 30% the viewport width as double tap zoom targets. r=emilio
https://hg.mozilla.org/integration/autoland/rev/d608a5951497
Add test. r=emilio
Flags: needinfo?(tnikkel)
Pushed by tnikkel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/19e0e1472a8c
Don't choose table cells smaller than 30% the viewport width as double tap zoom targets. r=emilio
https://hg.mozilla.org/integration/autoland/rev/744273c4719b
Add test. r=emilio
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Whiteboard: [proton-uplift]

Comment on attachment 9218324 [details]
Bug 1699147. Don't choose table cells smaller than 30% the viewport width as double tap zoom targets. r?emilio,botond

Beta/Release Uplift Approval Request

  • User impact if declined: double tap zoom too far in on table cells
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): just tweaks the heuristic for which element to zoom to
  • String changes made/needed:
Attachment #9218324 - Flags: approval-mozilla-beta?
Attachment #9210964 - Flags: approval-mozilla-beta?
Attachment #9219106 - Flags: approval-mozilla-beta?

Timothy, you asked for uplift 3 phabricator revisions but it seems that only 2 landed on central, the one adding the apz.double_tap.max_zoom_in_factor_change pref does not seem to be on m-c, is that intended? Thanks

Flags: needinfo?(tnikkel)

Oops! That was an earlier approach that we have decided not to pursue. I'll try to clear that make it obsolete. The form auto requests all patches and I forgot we had that one here.

Flags: needinfo?(tnikkel)
Attachment #9210964 - Flags: approval-mozilla-beta?
Attachment #9210964 - Attachment is obsolete: true

Comment on attachment 9218324 [details]
Bug 1699147. Don't choose table cells smaller than 30% the viewport width as double tap zoom targets. r?emilio,botond

Low risk, Approved for 89 beta 8. This was build on top of bug 1707802 which will be uplifted together, thanks.

Attachment #9218324 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9219106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: in-testsuite+

Hi Timothy,

I'm still encountering the issue mentioned in comment 0 on latest Firefox Nightly & Beta 8. It seems that the zoom level is still to high for the wikipedia case.

Screencast (Mozilla account needed).

Am I missing something ?

Flags: needinfo?(tnikkel)
Blocks: 1709614

Thanks.

I think the double tap on the globe is fine from that video. We zoom in more than Safari and we know that, and we like that decision. It fits the width of the globe.

The double tap on the percentages is less good, it's because the elements are a little more nested and so they avoid the heuristic I've used here. I filed bug 1709614 for that. I can extend the heuristic a bit to cover that.

Flags: needinfo?(tnikkel)

(In reply to Timothy Nikkel (:tnikkel) from comment #28)

I think the double tap on the globe is fine from that video. We zoom in more than Safari and we know that, and we like that decision. It fits the width of the globe.

I decided we could do better on the globe so I filed bug 1710034.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: