Changeset 210267 in webkit


Ignore:
Timestamp:
Jan 3, 2017 7:39:39 PM (7 years ago)
Author:
rniwa@webkit.org
Message:

label element with tabindex >= 0 is not focusable
​https://bugs.webkit.org/show_bug.cgi?id=102780
<rdar://problem/29796608>

Reviewed by Darin Adler.

Source/WebCore:

Fixed the bug by removing the override for HTMLLabelElement::isFocusable which always returned false.

This is a behavior from r5532 but it doesn't match the latest HTML specification or that of Chrome
and Firefox.

Also fixed an existing bug in HTMLLabelElement::focus and HTMLLegendElement::focus which focused
the associated form control when there is one even if the element itself is focusable. Without this fix,
traversing from control with shift+tab would break since focusing the label would move the focus back
to the input element inside the label element.

Finally, fixed a bug in HTMLLegendElement::focus that we can call inFocus without updating layout first.

The fix was inspired by ​https://chromium.googlesource.com/chromium/src/+/085ad8697b1be50c4f93e296797a25a43a79bcfb

Test: fast/events/focus-label-legend-elements-with-tabindex.html

  • html/HTMLLabelElement.cpp:

(WebCore::HTMLLabelElement::focus):
(WebCore::HTMLLabelElement::isFocusable): Deleted.

  • html/HTMLLabelElement.h:
  • html/HTMLLegendElement.cpp:

(WebCore::HTMLLegendElement::focus):

LayoutTests:

Added a regression test for traversing label and legend elements by tabbing.
A native merge of the blink fix would have regressed this for the label element
while the bug in the legend element had always existed.

Also added a regression test for focusing label and legend elements with tabindex.
We should be able to focus either element. New behavior matches that of Chrome.
Firefox moves the focus to the label element like we used to before this patch.

Also merge the test fix from ​https://chromium.googlesource.com/chromium/src/+/085ad8697b1be50c4f93e296797a25a43a79bcfb

  • fast/events/focus-label-legend-elements-expected.txt: Added.
  • fast/events/focus-label-legend-elements-with-tab-expected.txt: Added.
  • fast/events/focus-label-legend-elements-with-tab.html: Added.
  • fast/events/focus-label-legend-elements.html: Added.
  • fast/events/resources/tabindex-focus-blur-all-frame1.html:
  • fast/events/resources/tabindex-focus-blur-all-frame2.html:
  • fast/events/resources/tabindex-focus-blur-all.js:
  • fast/events/tabindex-focus-blur-all-expected.txt:
  • platform/ios-simulator-wk2/TestExpectations:
Location:
trunk
Files:
4 added
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r210266 r210267  
     12017-01-03  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        label element with tabindex >= 0 is not focusable
     4        https://bugs.webkit.org/show_bug.cgi?id=102780
     5        <rdar://problem/29796608>
     6
     7        Reviewed by Darin Adler.
     8
     9        Added a regression test for traversing label and legend elements by tabbing.
     10        A native merge of the blink fix would have regressed this for the label element
     11        while the bug in the legend element had always existed.
     12
     13        Also added a regression test for focusing label and legend elements with tabindex.
     14        We should be able to focus either element. New behavior matches that of Chrome.
     15        Firefox moves the focus to the label element like we used to before this patch.
     16
     17        Also merge the test fix from https://chromium.googlesource.com/chromium/src/+/085ad8697b1be50c4f93e296797a25a43a79bcfb
     18
     19        * fast/events/focus-label-legend-elements-expected.txt: Added.
     20        * fast/events/focus-label-legend-elements-with-tab-expected.txt: Added.
     21        * fast/events/focus-label-legend-elements-with-tab.html: Added.
     22        * fast/events/focus-label-legend-elements.html: Added.
     23        * fast/events/resources/tabindex-focus-blur-all-frame1.html:
     24        * fast/events/resources/tabindex-focus-blur-all-frame2.html:
     25        * fast/events/resources/tabindex-focus-blur-all.js:
     26        * fast/events/tabindex-focus-blur-all-expected.txt:
     27        * platform/ios-simulator-wk2/TestExpectations:
     28
    1292017-01-03  Tim Horton  <timothy_horton@apple.com>
    230

  • trunk/LayoutTests/fast/events/resources/tabindex-focus-blur-all-frame1.html

    r205858 r210267  
    155155<input type="radio" id="radioChoice1a" name="radio1" tabindex="0"><br>
    156156<label id="label1" tabindex="-1"><input type="radio" name="radio1">label for radioChoice B</label><br>
     157<label id="label2"><input type="radio" name="radio1">label for radioChoice C</label><br>
    157158<input type="file" id="file1" tabindex="2"><br>
    158159input type="hidden"<input type="hidden" id="hidden1" tabindex="-1"><br>

  • trunk/LayoutTests/fast/events/resources/tabindex-focus-blur-all-frame2.html

    r205858 r210267  
    155155<input type="radio" id="radioChoice3a" name="radio3"><br>
    156156<label id="label3" tabindex="2"><input type="radio" name="radio3" tabindex="0">label for radio button</label><br>
     157<label id="label4"><input type="radio" name="radio3" tabindex="0">another label for radio button</label><br>
    157158<input type="file" id="file3" tabindex="-1"><br>
    158159input type="hidden"<input type="hidden" id="hidden3" tabindex="3"><br>

  • trunk/LayoutTests/fast/events/resources/tabindex-focus-blur-all.js

    r147676 r210267  
    5656    homeBase[0].focus();
    5757
    58     var resultSummary = focusCount+" focus / "+blurCount+" blur events dispatched, and should be 333 / 333 ";
     58    var resultSummary = focusCount+" focus / "+blurCount+" blur events dispatched, and should be 336 / 336 ";
    5959    resultSummary += (focusCount==blurCount) ? "<span style='color:green'>PASSED</span><br>" : "<span style='color:red'>FAILED</span><br>";
    6060    resultSummary += "Total of "+failedTestCount+" focus test(s) failed.";
    … …  
    102102{
    103103    var elemThatShouldFocus = null;
    104     var OKtoFocusOtherElement = false;
     104    var shouldFocusOtherElement = false;
    105105    focusedElem = null;
    106106
    … …  
    118118        elemThatShouldFocus = null;
    119119
    120     if (tagNamesTransferFocused.find(elem.tagName)) {
    121         elemThatShouldFocus = null;
    122         OKtoFocusOtherElement = true;
    123     }
     120    if (tagNamesTransferFocused.find(elem.tagName) && elemThatShouldFocus == null)
     121        shouldFocusOtherElement = true;
    124122
    125123    elem.focus();
    126     if (elemThatShouldFocus == focusedElem || (!elemThatShouldFocus && OKtoFocusOtherElement))
     124    if ((!shouldFocusOtherElement && elemThatShouldFocus == focusedElem) || (shouldFocusOtherElement && elem != focusedElem))
    127125        printToConsole("<"+elem.tagName+"> "+elem.id+" PASSED focus test");
    128126    else {

  • trunk/LayoutTests/fast/events/tabindex-focus-blur-all-expected.txt

    r95381 r210267  
    1 333 focus / 333 blur events dispatched, and should be 333 / 333 PASSED
     1336 focus / 336 blur events dispatched, and should be 336 / 336 PASSED
    22Total of 0 focus test(s) failed. PASSED

  • trunk/LayoutTests/platform/ios-simulator-wk2/TestExpectations

    r210143 r210267  
    17671767fast/shadow-dom/fullscreen-in-slot-fullscreenElement.html [ Skip ]
    17681768fast/shadow-dom/fullscreen-in-slot-webkitCurrentFullScreenElement.html [ Skip ]
     1769fast/events/focus-label-legend-elements-with-tab.html [ Skip ]
    17691770
    17701771# No touch events

  • trunk/Source/WebCore/ChangeLog

    r210266 r210267  
     12017-01-03  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        label element with tabindex >= 0 is not focusable
     4        https://bugs.webkit.org/show_bug.cgi?id=102780
     5        <rdar://problem/29796608>
     6
     7        Reviewed by Darin Adler.
     8
     9        Fixed the bug by removing the override for HTMLLabelElement::isFocusable which always returned false.
     10
     11        This is a behavior from r5532 but it doesn't match the latest HTML specification or that of Chrome
     12        and Firefox.
     13
     14        Also fixed an existing bug in HTMLLabelElement::focus and HTMLLegendElement::focus which focused
     15        the associated form control when there is one even if the element itself is focusable. Without this fix,
     16        traversing from control with shift+tab would break since focusing the label would move the focus back
     17        to the input element inside the label element.
     18
     19        Finally, fixed a bug in HTMLLegendElement::focus that we can call inFocus without updating layout first.
     20
     21        The fix was inspired by https://chromium.googlesource.com/chromium/src/+/085ad8697b1be50c4f93e296797a25a43a79bcfb
     22
     23        Test: fast/events/focus-label-legend-elements-with-tabindex.html
     24
     25        * html/HTMLLabelElement.cpp:
     26        (WebCore::HTMLLabelElement::focus):
     27        (WebCore::HTMLLabelElement::isFocusable): Deleted.
     28        * html/HTMLLabelElement.h:
     29        * html/HTMLLegendElement.cpp:
     30        (WebCore::HTMLLegendElement::focus):
     31
    1322017-01-03  Tim Horton  <timothy_horton@apple.com>
    233

  • trunk/Source/WebCore/html/HTMLLabelElement.cpp

    r206332 r210267  
    5757{
    5858    return adoptRef(*new HTMLLabelElement(tagName, document));
    59 }
    60 
    61 bool HTMLLabelElement::isFocusable() const
    62 {
    63     return false;
    6459}
    6560
    … …  
    151146}
    152147
    153 void HTMLLabelElement::focus(bool, FocusDirection direction)
     148void HTMLLabelElement::focus(bool restorePreviousSelection, FocusDirection direction)
    154149{
    155     // to match other browsers, always restore previous selection
     150    if (document().haveStylesheetsLoaded()) {
     151        document().updateLayout();
     152        if (isFocusable()) {
     153            // The value of restorePreviousSelection is not used for label elements as it doesn't override updateFocusAppearance.
     154            Element::focus(restorePreviousSelection, direction);
     155            return;
     156        }
     157    }
     158
     159    // To match other browsers, always restore previous selection.
    156160    if (auto* element = control())
    157161        element->focus(true, direction);

  • trunk/Source/WebCore/html/HTMLLabelElement.h

    r206332 r210267  
    4040    HTMLLabelElement(const QualifiedName&, Document&);
    4141
    42     bool isFocusable() const final;
    43 
    4442    void accessKeyAction(bool sendMouseEvents) final;
    4543

  • trunk/Source/WebCore/html/HTMLLegendElement.cpp

    r177996 r210267  
    5858}
    5959
    60 void HTMLLegendElement::focus(bool, FocusDirection direction)
     60void HTMLLegendElement::focus(bool restorePreviousSelection, FocusDirection direction)
    6161{
    62     if (isFocusable())
    63         Element::focus(true, direction);
    64        
    6562    // To match other browsers' behavior, never restore previous selection.
    66     if (HTMLFormControlElement* control = associatedControl())
     63    if (document().haveStylesheetsLoaded()) {
     64        document().updateLayoutIgnorePendingStylesheets();
     65        if (isFocusable()) {
     66            Element::focus(restorePreviousSelection, direction);
     67            return;
     68        }
     69    }
     70    if (auto* control = associatedControl())
    6771        control->focus(false, direction);
    6872}
Note: See TracChangeset for help on using the changeset viewer.