Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15238002:
Disallow tabsToLinks setting to prevent tabbing to anchor with tabIndex. (Closed)
DescriptionAllow tabbing to anchors with explicit tabIndex even when tabsToLinks is disabled
With tabsToLinks setting enabled, browser focuses all links on pressing tab.
With it disabled, it does not focus any. This patch changes behavior when the
option is disabled so that links with explicit tabIndex are focusable. When
author has specified tabIndex, he most likely wants the element to be keyboard
accessible.
This option is by default enabled but only Mac exposes it in the settings so
it only affects Mac users who have changed default settings.
BUG=240678
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=157684
Patch Set 1 #Patch Set 2 : Tests update #
Total comments: 6
Patch Set 3 : Address review issues #Patch Set 4 : rebase on master #Patch Set 5 : Update tests #Patch Set 6 : More minimal changes to existing test & add new test #Patch Set 7 : Actually add new test #
Total comments: 1
Patch Set 8 : Update new test to use modern framework #Patch Set 9 : rebase #Patch Set 10 : Add missing line break in frame-tab-focus. Remove accidentally commited line in tab-focus-anchor-ex… #Patch Set 11 : And update expected result after removing accidentally added line #
Messages
Total messages: 40 (0 generated)
I'm not familiar with the "tabs to links" setting?
On 2013/05/16 22:44:23, eseidel wrote: > I'm not familiar with the "tabs to links" setting? Did I select you as a reviewer or did this happen automatically? I'm pretty new to this system so bare with me. :) But anyways, this setting is by default enabled on Windows and Linux and when enabled, allows to focus every link element by pressing tab. Regardless of whether it has explicit tabIndex attribute or not. When this setting is disabled (default on Mac), Chromium never includes links in tab navigation. This patch changes it so that links that have explicit tabIndex can always be navigated to using 'tab', regardless of this setting. If user explicitly set tabIndex on the link, he apparently wants it to be navigable. Not allowing that seems wrong. BTW. I think this setting is only exposed in the UI (chrome:settings) on Mac. On other platforms it's not possible to change it without recompiling (I think).
eseidel, do you think rchl's comment covers your questions?
It seems you want an AX review. :)
if the AX peeps like it, it's fine with me.
What platforms will this affect, and how do other browsers behave? I'm only aware of Safari having this preference on Mac. Please rebase and run try jobs again, I want to make sure no other tests are affected. https://chromiumcodereview.appspot.com/15238002/diff/2001/LayoutTests/fast/ev... File LayoutTests/fast/events/tab-focus-anchor.html (right): https://chromiumcodereview.appspot.com/15238002/diff/2001/LayoutTests/fast/ev... LayoutTests/fast/events/tab-focus-anchor.html:6: function print(result) I liked it better with pass() and fail(). https://chromiumcodereview.appspot.com/15238002/diff/2001/LayoutTests/fast/ev... LayoutTests/fast/events/tab-focus-anchor.html:35: <p><a onfocus="print('FAIL')">Not focusable (given the WebKitTabToLinks pref is set to false)</a> This one is not focusable no matter what the value of the pref, so the comment is unclear. https://chromiumcodereview.appspot.com/15238002/diff/2001/Source/core/html/HT... File Source/core/html/HTMLAnchorElement.cpp (right): https://chromiumcodereview.appspot.com/15238002/diff/2001/Source/core/html/HT... Source/core/html/HTMLAnchorElement.cpp:143: // If tab index set explicitly, decide focus regardless of the tabsToLinks setting. Well, technically you're also skipping the test that document()->page() exists, and the canvas subtree and hasNonEmptyBoundingBox tests. I think bypassing the canvas subtree and hasNonEmptyBoundingBox tests make sense, but you should mention that in the comment, something more like: // If the tab index is set explicitly, ignore the tabsToLinks // setting and empty bounding box check. Is there ever a case when the page doesn't exist, or is that just a defensive test?
08/01 21:48:15, Dominic Mazzoni wrote: > What platforms will this affect, and how do other > browsers behave? I'm only aware of Safari having this > preference on Mac. This should only affect Mac because only Mac has tabsToLinks disabled. (All below applies when this option is disabled because enabled state should be unaffected) Currently (before this change) chromium is consistent with Safari. It will not focus any links. Even those with explicit tab-index. With the fix, links with explicit tab-index will be focused on tabbing. Opera 12: On pressing tab, focuses links with tabindex only. Opera 15: Same (includes this fix). Firefox: Focuses all links by default. Has option to affect that but doesn't seem to support behavior of this patch. http://kb.mozillazine.org/Accessibility.tabfocus Chrome: Focuses all links by default on Windows. None by default on Mac IE: Focuses all links by default. So asically this change aligns with Opera which is maybe not convincing argument given the market share but I think it makes sense if you think about it. > Please rebase and run try jobs again, I want to make > sure no other tests are affected. Will do that later, when I figure out how the system works. And also files have moved so that doesn't help. Will also address your comments later.
https://chromiumcodereview.appspot.com/15238002/diff/2001/LayoutTests/fast/ev... File LayoutTests/fast/events/tab-focus-anchor.html (right): https://chromiumcodereview.appspot.com/15238002/diff/2001/LayoutTests/fast/ev... LayoutTests/fast/events/tab-focus-anchor.html:6: function print(result) On 2013/08/01 21:48:15, Dominic Mazzoni wrote: > I liked it better with pass() and fail(). Done. https://chromiumcodereview.appspot.com/15238002/diff/2001/LayoutTests/fast/ev... LayoutTests/fast/events/tab-focus-anchor.html:35: <p><a onfocus="print('FAIL')">Not focusable (given the WebKitTabToLinks pref is set to false)</a> On 2013/08/01 21:48:15, Dominic Mazzoni wrote: > This one is not focusable no matter what the value of the pref, so the comment > is unclear. Done. https://chromiumcodereview.appspot.com/15238002/diff/2001/Source/core/html/HT... File Source/core/html/HTMLAnchorElement.cpp (right): https://chromiumcodereview.appspot.com/15238002/diff/2001/Source/core/html/HT... Source/core/html/HTMLAnchorElement.cpp:143: // If tab index set explicitly, decide focus regardless of the tabsToLinks setting. On 2013/08/01 21:48:15, Dominic Mazzoni wrote: > Well, technically you're also skipping the test that document()->page() exists, > and the canvas subtree and hasNonEmptyBoundingBox tests. > > I think bypassing the canvas subtree and hasNonEmptyBoundingBox tests make > sense, but you should mention that in the comment, something more like: > > // If the tab index is set explicitly, ignore the tabsToLinks > // setting and empty bounding box check. > Is there ever a case when the page doesn't exist, or is that just > a defensive test? I can't say for sure but the first "!isLink()" condition that I've removed could also return true without checking if page existed so I guess it was fine. It seems like the 'page' variable is used solely for the tabsToLinks check and probably the code is just being defensive.
Someone from Google must submit try jobs. I don't have permissions to do it.
On 2013/08/20 18:03:33, rchl wrote: > Someone from Google must submit try jobs. I don't have permissions to do it. Can we try to move it forward?
Sorry, I missed that this was updated. I just added try jobs. Feel free to nag if you don't get a response within a couple of (normal working) days. It's okay to politely nag.
On 2013/09/05 14:54:45, Dominic Mazzoni wrote: > Sorry, I missed that this was updated. I just added try jobs. > > Feel free to nag if you don't get a response within a couple of (normal working) > days. It's okay to politely nag. Not sure but looks like I might have to rebase to make bots happy. I've run git cl rebase but it seems to be stuck doing nothing for the past hour. Oh well, will keep trying.
I've rebased and updated patch description, clarifying few things. Could you please try requesting bots again? BTW. It seems like the default value of tabsToLinks option (kWebkitTabsToLinks pref) is "enabled" for both Mac and Windows. I'm not sure if that has changed while the review was in progress or I have checked it wrong before. So that means that this change should only affect Mac users that have changed default option (the option is only exposed on Mac).
webkit_tests webkit_tests 291 fixable (0 skipped) failed 2 tab-focus-anchor.html - my fault, forgot to update expected result. frame-tab-focus.html - not sure I quite understand this one. It seems like it expects results that would normally only happen with tabsToLinks disabled but I don't see any change of the default pref value in this test.
On 2013/09/06 13:26:27, Rafał Chłodnicki wrote: > frame-tab-focus.html - not sure I quite understand this one. It seems like it > expects results that would normally only happen with tabsToLinks disabled but I > don't see any change of the default pref value in this test. I thought that an explicit tabIndex overrides the tabsToLinks setting?
On 2013/09/09 05:51:48, Dominic Mazzoni wrote: > On 2013/09/06 13:26:27, Rafał Chłodnicki wrote: > > frame-tab-focus.html - not sure I quite understand this one. It seems like it > > expects results that would normally only happen with tabsToLinks disabled but > I > > don't see any change of the default pref value in this test. > > I thought that an explicit tabIndex overrides the tabsToLinks setting? Yes, but the test expects that links are not focused when tabbing forward for the first time[1] which is weird because without my fix that would only be the case when tabsToLinks is disabled (and it's not by default). When I open the test manually in official Chrome and Chrome Canary builds on Windows the test fails as I would expect because tabsToLinks is enabled. So somehow, in the framework, tabsToLinks seems to be off. I wonder if it's because some other test before changed the default and it carried over to this test? Is that even possible or each test gets a clean slate? Anyway, I can of course fix the test with the assumption that tabsToLinks is off. Understanding why this is the case would be a nice bonus though. [1] https://chromium.googlesource.com/chromium/blink/+/master/LayoutTests/fast/ev...
Apart from that, test is not testing what it describes. It uses synthetic events to trigger TAB key first and on the second round, it supposedly triggers OPTION+TAB to include all the links in the focus order. The thing is, expected results don't include A elements in neither case so basically synthetic OPTION+TAB key event doesn't do anything different from the plain TAB key event. Should I just remove the part that tests OPTION+TAB? It's really confusing for the test to say something different in the manual instruction and have different exceptions in the *-expected.txt file.
To describe the changes I've done in the tests: In frame-tab-focus.html: - Removed tests for tabbing with "option" key as these clearly didn't work as expected. Links were not including in the tabbing order. I believe Chrome does not even support option+tab to cycle through all links (I've tried on Mac with "tabsToLink" disabled). This must be safari specific feature. Somehow probably updated the test to follow Chrome behavior, making it quite confusing if read manually. - Added one link without tabindex to make test more "interesting" - make sure that it's not focused. - Removed green background that was set on document focus. It was confusing, making the test look like it passed while actual pass condition has to be checked by manually inspecting output (or comparing with *-expected.txt). In tab-focus-anchor.html updated expected condition.
On 2013/09/09 07:59:50, rchl wrote:
> Yes, but the test expects that links are not focused when tabbing forward for
> the first time[1] which is weird because without my fix that would only be the
> case when tabsToLinks is disabled (and it's not by default).
Looking through the code, it appears to me that tabsToLinks is false by default
in TestRunner. (See WebPreferences.cpp)
Here's what to call to change the value of the pref from within the test:
testRunner.overridePreference('WebKitTabToLinksPreferenceKey', true);
Maybe you could add a new test that tabs through the same elements twice - once
with tabsToLinks false, and once with tabsToLinks true. Include a mix of
tabIndex and no tabIndex.
Outside of that I'd prefer you make minimal changes to existing tests. Removing
the green background is good, updating the expectations when we're happy with
the new behavior is good - but I'd rather see you add a new test rather than
add/remove tabIndex from an existing tabbing test.
On 2013/09/09 14:47:56, Dominic Mazzoni wrote: > Outside of that I'd prefer you make minimal changes to existing tests. Removing > the green background is good, updating the expectations when we're happy with > the new behavior is good - but I'd rather see you add a new test rather than > add/remove tabIndex from an existing tabbing test. OK, I can revert most changes from this test and only update expectations. That means its description will not really match expected results though. As for creating new test... tab-focus-anchor.html already tests WebKitTabToLinksPreferenceKey=false case. I can add new test that tests with it enabled.
On 2013/09/09 15:31:03, rchl wrote: > On 2013/09/09 14:47:56, Dominic Mazzoni wrote: > > Outside of that I'd prefer you make minimal changes to existing tests. > Removing > > the green background is good, updating the expectations when we're happy with > > the new behavior is good - but I'd rather see you add a new test rather than > > add/remove tabIndex from an existing tabbing test. > > OK, I can revert most changes from this test and only update expectations. That > means its description will not really match expected results though. Please update the description. Fixing things that are broken is still fine, but normally don't change the contents of a test just to cover different things - the original test may have actually been that way to catch a particular regression. > As for creating new test... tab-focus-anchor.html already tests > WebKitTabToLinksPreferenceKey=false case. I can add new test that tests with it > enabled. Thanks. It looks like we don't have much coverage for the true case, which is the Chrome default.
On 2013/09/09 15:45:06, Dominic Mazzoni wrote: > > OK, I can revert most changes from this test and only update expectations. > That > > means its description will not really match expected results though. > > Please update the description. Fixing things that are broken is still fine, but > normally don't change the contents of a test just to cover different things - > the > original test may have actually been that way to catch a particular regression. If I'd just update the description to match reality it'd have to be something like: "Press tab 7 times and Shift+tab 7 times to move focus through 7 elements. Then press option+tab and shift+option+tab which should move focus through 11 elements." That doesn't make quite sense. Original test meant to test that TAB does not put focus on links and option+tab does (which as I've said, never really worked). How about I change the test to move the focus through all 11 elements forth and back once and then do the same with option+tab (With expectation that there is no difference between those)?
OK, I just took a closer look at the test history. The test is 7 years old in WebKit. The Blink expectations look nothing like the WebKit expectations - they were rebaselined without really checking the option-tabbing behavior. Argh. To capture the spirit of the original test, maybe that test should flip the WebKitTabToLinksPreferenceKey flag and then tab through again, in place of option-tabbing. Then the original expectations should be what we get (modulo a few minor string differences). For reference, here's the original expectations from WebKit: https://trac.webkit.org/browser/trunk/LayoutTests/fast/events/frame-tab-focus... Ideally we should get something pretty close to that.
On 2013/09/10 21:03:05, Dominic Mazzoni wrote: > To capture the spirit of the original test, maybe that test should flip the > WebKitTabToLinksPreferenceKey flag and then tab through again, in place of > option-tabbing. Then the original expectations should be what we get (modulo a > few minor string differences). You might be forgetting that my change makes the links with tabindex focusable. Given that all the links in this test have tabindex, results will be the same regardless of the setting. Unless I add some links without tabindex but then we are back to doing less trivial changes to the test. I staid by my suggestion (maybe slightly modified) - focus through all the elements forth and back once with tabToLinks disabled and repeat with tabToLinks enabled. Expect same results.
On 2013/09/10 21:09:54, rchl wrote: > You might be forgetting that my change makes the links with tabindex focusable. Oh, that's right, these ones have explicit tabindex. I guess there's no way to salvage that particular test. > I staid by my suggestion (maybe slightly modified) - focus through all the > elements forth and back once with tabToLinks disabled and repeat with tabToLinks > enabled. Expect same results. OK, that works. One other question - doesn't have to be part of this change, but what happens if you flip the tabsToLinks preference so that it matches the browser default, rather than the rare Mac-only case? How many tests would need to be modified?
https://chromiumcodereview.appspot.com/15238002/diff/32001/LayoutTests/fast/e... File LayoutTests/fast/events/tab-focus-anchor-tab-to-links.html (right): https://chromiumcodereview.appspot.com/15238002/diff/32001/LayoutTests/fast/e... LayoutTests/fast/events/tab-focus-anchor-tab-to-links.html:6: function pass() One last thing - since this is a new test it'd be nice if it followed the basic template of more recent tests. If you just search for any tests added in the last year or two in this directory that should be fine. Something that uses js-test-pre and shouldBe() and debug(), rather than manual pass() and fail().
On 2013/09/10 21:33:14, Dominic Mazzoni wrote: > One other question - doesn't have to be part of this change, but what happens if > you flip the tabsToLinks preference so that it matches the browser default, > rather than the rare Mac-only case? How many tests would need to be modified? I've corrected myself in comment #16 saying that it's actually enabled for Mac too. So default doesn't match any platform actually. I have not checked if flipping default would cause any additional failures. I can try that after being done with this fix.
On 2013/09/10 21:40:14, rchl wrote: > On 2013/09/10 21:33:14, Dominic Mazzoni wrote: > > One other question - doesn't have to be part of this change, but what happens > if > > you flip the tabsToLinks preference so that it matches the browser default, > > rather than the rare Mac-only case? How many tests would need to be modified? > > I've corrected myself in comment #16 saying that it's actually enabled for Mac > too. So default doesn't match any platform actually. > I have not checked if flipping default would cause any additional failures. I > can try that after being done with this fix.
(Previous comment contains no input - accidental submit) I hate frame-tab-focus.html. I can't make its description have sense both for manual and automated testing. One reason is that the synthetic event behave differently from manual tabbing - synthetic event skips first link at the very beginning. And also user can't really change the pref so that makes manual testing difficult.
lgtm
On 2013/09/10 22:34:28, rchl wrote: > I hate frame-tab-focus.html. I can't make its description have sense both for > manual and automated testing. One reason is that the synthetic event behave > differently from manual tabbing - synthetic event skips first link at the very > beginning. And also user can't really change the pref so that makes manual > testing difficult. Agreed, I'm now convinced this test just isn't useful anymore.
Darn, someone changed the code... I'm submitting rebase but haven't put much thought into it. Just tried to recreate previous logic more or less. Not tested because it's 1am here and I'm going to sleep now. Could you please at least trigger bots to see where we are standing now?
Try bots running again. Hopefully we're close. On Tue, Sep 10, 2013 at 4:06 PM, <rchl@opera.com> wrote: > Darn, someone changed the code... > > I'm submitting rebase but haven't put much thought into it. Just tried to > recreate previous logic more or less. > Not tested because it's 1am here and I'm going to sleep now. > Could you please at least trigger bots to see where we are standing now? > > > https://chromiumcodereview.**appspot.com/15238002/<https://chromiumcodereview... > To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
Lets try bots again please. :)
On 2013/09/11 20:06:11, rchl wrote: > Lets try bots again please. :) Thanks. Linux failure doesn't look related.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rchl@opera.com/15238002/74001
Message was sent while issue was closed.
Change committed as 157684 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
