Summary: | AX: VoiceOver iframe scrolling focus jumping bug | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Paul J. Adam <paul> | ||||||||||||||
Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Blocker | CC: | aboxhall, apinheiro, cfleizach, commit-queue, dhbollinger, dmazzoni, ews-watchlist, jcraig, jdiggs, n_wang, rniwa, samuel_white, simon.fraser, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | Safari 10 | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=186956 | ||||||||||||||||
Attachments: |
|
Description
Paul J. Adam
2017-09-08 10:11:31 PDT
Created attachment 339895 [details]
patch
Comment on attachment 339895 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=339895&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:3000 > + LayoutRect objectRect = isScrollView() ? elementRect() : boundingBoxRect(); Should we implement bounding box rect for scroll view? (In reply to chris fleizach from comment #3) > Comment on attachment 339895 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339895&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:3000 > > + LayoutRect objectRect = isScrollView() ? elementRect() : boundingBoxRect(); > > Should we implement bounding box rect for scroll view? I think boundingBoxRect() should only be available when there's a renderer Comment on attachment 339895 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=339895&action=review > LayoutTests/accessibility/scroll-to-make-visible-iframe-offscreen.html:11 > +<iframe id="frame" src="data:text/html,<body><button id='upper_target'>Upper Target</button><div style='border: 1px solid #000; height: 5000px;'>5000-pixel box</div><button id='lower_target'>Lower Target</button></body>"></iframe> can we also test a iframe inside an iframe Comment on attachment 339895 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=339895&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:3027 > IntRect newSubfocus = subfocus; > IntRect newElementRect = snappedIntRect(elementRect()); > - IntRect scrollParentRect = snappedIntRect(scrollParent->elementRect()); > newSubfocus.move(newElementRect.x(), newElementRect.y()); > - newSubfocus.move(-scrollParentRect.x(), -scrollParentRect.y()); > We have a family of helper functions for this kind of thing: Widget::convertToContainingView, convertToRootView etc. Please always use those rather than trying to hand-roll coordinate conversion code, since they know about things like scroll view insets and headers/footers. > Source/WebCore/accessibility/AccessibilityObject.cpp:3028 > // Recursively make sure the scroll parent itself is visible. Why isn't this just using RenderLayer::scrollRectToVisible() which knows how to do all this scrolling? Comment on attachment 339895 [details] patch Attachment 339895 [details] did not pass win-ews (win): Output: http://webkit-queues.webkit.org/results/7618051 New failing tests: accessibility/scroll-to-make-visible-iframe-offscreen.html http/tests/preload/onload_event.html http/tests/security/canvas-remote-read-remote-video-localhost.html Created attachment 339911 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Created attachment 339916 [details]
patch
- Use RenderLayer::scrollRectToVisible()
- Added more test case
Comment on attachment 339916 [details] patch Attachment 339916 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/7619265 New failing tests: http/tests/security/XFrameOptions/x-frame-options-deny-multiple-clients.html Created attachment 339925 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 339916 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=339916&action=review > Source/WebCore/accessibility/AccessibilityObject.cpp:2992 > + renderer->scrollRectToVisible(SelectionRevealMode::Reveal, absoluteBounds, insideFixed, ScrollAlignment::alignCenterIfNotVisible, ScrollAlignment::alignCenterIfNotVisible); should we go with align center or align top? (In reply to chris fleizach from comment #12) > Comment on attachment 339916 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=339916&action=review > > > Source/WebCore/accessibility/AccessibilityObject.cpp:2992 > > + renderer->scrollRectToVisible(SelectionRevealMode::Reveal, absoluteBounds, insideFixed, ScrollAlignment::alignCenterIfNotVisible, ScrollAlignment::alignCenterIfNotVisible); > > should we go with align center or align top? I think previously we explicitly center the rect. Created attachment 340008 [details]
patch
Found an issue where renderTextElement is not being scrolled. Added a test case for that.
Comment on attachment 340008 [details] patch Clearing flags on attachment: 340008 Committed r231628: <https://trac.webkit.org/changeset/231628> All reviewed patches have been landed. Closing bug. |