[Double-tap zoom] The zoom in level is too high for some page (table) elements
Categories
(Core :: Panning and Zooming, defect, P2)
Tracking
()
People
(Reporter: emilghitta, Assigned: tnikkel)
References
Details
(Whiteboard: [proton-uplift])
Attachments
(3 files, 1 obsolete file)
877.80 KB,
video/mp4
|
Details | |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
48 bytes,
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
|
Details | Review |
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
- Launch Firefox.
- Access the following link.
- 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.
Comment 1•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 2•3 years ago
|
||
: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?
Updated•3 years ago
|
Assignee | ||
Comment 3•3 years ago
|
||
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.
Comment 4•3 years ago
|
||
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.
Assignee | ||
Comment 5•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 6•3 years ago
•
|
||
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.
Comment 7•3 years ago
|
||
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.
Assignee | ||
Comment 8•3 years ago
|
||
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.
Assignee | ||
Comment 9•3 years ago
|
||
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.
Comment 10•3 years ago
|
||
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 :)
Comment 11•3 years ago
|
||
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.
Updated•3 years ago
|
Assignee | ||
Comment 12•3 years ago
|
||
We're not sure if we want to land that specific patch.
Comment 13•3 years ago
•
|
||
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.
Assignee | ||
Comment 14•3 years ago
|
||
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.
Assignee | ||
Comment 15•3 years ago
|
||
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.
Assignee | ||
Comment 16•3 years ago
|
||
Assignee | ||
Updated•3 years ago
|
Comment 17•3 years ago
|
||
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
Comment 18•3 years ago
|
||
Backed out for causing build bustages on DoubleTapToZoom.cpp:
https://hg.mozilla.org/integration/autoland/rev/5e002f2f19f72a2993c03bff4a83e5299c61329a
Failure log: https://treeherder.mozilla.org/logviewer?job_id=338271519&repo=autoland&lineNumber=15143
Assignee | ||
Updated•3 years ago
|
Comment 19•3 years ago
|
||
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
Comment 20•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/19e0e1472a8c
https://hg.mozilla.org/mozilla-central/rev/744273c4719b
Comment 21•3 years ago
|
||
Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 22•3 years ago
|
||
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:
Assignee | ||
Updated•3 years ago
|
Comment 23•3 years ago
|
||
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
Assignee | ||
Comment 24•3 years ago
|
||
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.
Assignee | ||
Updated•3 years ago
|
Updated•3 years ago
|
Comment 25•3 years ago
|
||
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.
Updated•3 years ago
|
Comment 26•3 years ago
|
||
bugherder uplift |
Updated•3 years ago
|
Reporter | ||
Comment 27•3 years ago
|
||
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 ?
Assignee | ||
Comment 28•3 years ago
|
||
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.
Assignee | ||
Comment 29•3 years ago
|
||
(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.
Description
•