⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 248784 in webkit


Ignore:
Timestamp:
Aug 16, 2019, 12:43:34 PM (7 years ago)
Author:
rniwa@webkit.org
Message:

Split tabIndex computation for DOM and the rest of WebCore
​https://bugs.webkit.org/show_bug.cgi?id=200806

Reviewed by Chris Dumez.

Source/WebCore:

This patch renames Element::tabIndex to Element::tabIndexForBindings and migrates its usage in
WebCore outside JS bindings code to: tabIndexSetExplicitly, which now returns Optional<int>,
and shouldBeIgnoredInSequentialFocusNavigation which returns true whenever the old tabIndex
function used to return -1.

Instead of overriding Element::tabIndex, each subclass of element now overrides defaultTabIndex
corresponding to the concept of the default value of tabIndex IDL attribute defined at:
​https://html.spec.whatwg.org/multipage/interaction.html#the-tabindex-attribute

No new tests since there should be no observable behavior change.

  • dom/Element.cpp:

(WebCore::Element::tabIndexSetExplicitly const): Now returns Optional<int> instead of bool.
(WebCore::Element::defaultTabIndex const): Added. Return -1 here. HTMLElement and SVGElement
manually override tabIndex to implement this behavior. Now MathMLElement overrides this function
to return 0 instead, which is arguably a bug.
(WebCore::Element::supportsFocus const): Convert Optional<int> to bool.
(WebCore::Element::tabIndexForBindings const): Renamed from tabIndex. Migrated the code in
HTMLElement::tabIndex and SVGElement::tabIndex here. Note all overrides of HTMLElement::tabIndex
and SVGElement::tabIndex below were skipping supportsFocus check and using 0 as the default value.
This is now accomplished by having an explicit check defaultTabIndex returning 0. MathMLElement
overrides defaultTabIndex so it continues to use the old logic. All this complexity should go away
in webkit.org/b/199606.
(WebCore::Element::setTabIndexForBindings): Renamed from setTabIndex.
(WebCore::Element::isKeyboardFocusable const): Checks shouldBeIgnoredInSequentialFocusNavigation
in lieu of calling Element::tabIndexForBindings.

  • dom/Element.h:

(WebCore::Element::shouldBeIgnoredInSequentialFocusNavigation const): Added. Returns true if the
old implementation of Element::tabIndex would have returned -1 due to supportsFocus returning false.

  • dom/ElementRareData.h:

(WebCore::ElementRareData::tabIndex const): Made this function return Optional<int>. Note that
ElementRareData continue to store a bit field and int for more efficient packing.

  • html/HTMLAnchorElement.cpp:

(WebCore::HTMLAnchorElement::defaultTabIndex const): Replaced tabIndex.

  • html/HTMLAnchorElement.h:
  • html/HTMLAreaElement.cpp:

(WebCore::HTMLAreaElement::isFocusable const):

  • html/HTMLElement.cpp:

(WebCore::HTMLElement::tabIndex const): Deleted. The logic is now in Element::tabIndex itself.

  • html/HTMLElement.h:
  • html/HTMLElement.idl:
  • html/HTMLFormControlElement.cpp:

(WebCore::HTMLFormControlElement::defaultTabIndex const): Replaced tabIndex.

  • html/HTMLFormControlElement.h:
  • mathml/MathMLElement.cpp:

(WebCore::MathMLElement::defaultTabIndex const): Replaced tabIndex. This is probably a bug since
this would put every MathML element in the sequential navigation order regardless of whether it
has tabIndex set or not.

  • mathml/MathMLElement.h:
  • page/FocusController.cpp:

(WebCore::tabIndexForElement): Added. Computes the "effective" tab index FocusController uses.
(WebCore::shadowAdjustedTabIndex):
(WebCore::nextElementWithGreaterTabIndex): This code should use shadowAdjustedTabIndex instead
but keeping the old behavior for now.

  • svg/SVGAElement.cpp:

(WebCore::SVGAElement::defaultTabIndex const): Replaced tabIndex.

  • svg/SVGAElement.h:
  • svg/SVGElement.cpp:

(WebCore::SVGElement::tabIndex const): Deleted. The logic is now in Element::tabIndex itself.

  • svg/SVGElement.h:

(WebCore::SVGElement::hasTagName const):

  • svg/SVGElement.idl:

Source/WebKit:

  • WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMHTMLElement.cpp:

(webkit_dom_html_element_get_tab_index):
(webkit_dom_html_element_set_tab_index):

Source/WebKitLegacy/mac:

  • DOM/DOMHTMLElement.mm:

(-[DOMHTMLElement tabIndex]):
(-[DOMHTMLElement setTabIndex:]):

Location:
trunk/Source
Files:
24 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r248783 r248784  
     12019-08-16  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Split tabIndex computation for DOM and the rest of WebCore
     4        https://bugs.webkit.org/show_bug.cgi?id=200806
     5
     6        Reviewed by Chris Dumez.
     7
     8        This patch renames Element::tabIndex to Element::tabIndexForBindings and migrates its usage in
     9        WebCore outside JS bindings code to: tabIndexSetExplicitly, which now returns Optional<int>,
     10        and shouldBeIgnoredInSequentialFocusNavigation which returns true whenever the old tabIndex
     11        function used to return -1.
     12
     13        Instead of overriding Element::tabIndex, each subclass of element now overrides defaultTabIndex
     14        corresponding to the concept of the default value of tabIndex IDL attribute defined at:
     15        https://html.spec.whatwg.org/multipage/interaction.html#the-tabindex-attribute
     16
     17        No new tests since there should be no observable behavior change.
     18
     19        * dom/Element.cpp:
     20        (WebCore::Element::tabIndexSetExplicitly const): Now returns Optional<int> instead of bool.
     21        (WebCore::Element::defaultTabIndex const): Added. Return -1 here. HTMLElement and SVGElement
     22        manually override tabIndex to implement this behavior. Now MathMLElement overrides this function
     23        to return 0 instead, which is arguably a bug.
     24        (WebCore::Element::supportsFocus const): Convert Optional<int> to bool.
     25        (WebCore::Element::tabIndexForBindings const): Renamed from tabIndex. Migrated the code in
     26        HTMLElement::tabIndex and SVGElement::tabIndex here. Note all overrides of HTMLElement::tabIndex
     27        and SVGElement::tabIndex below were skipping supportsFocus check and using 0 as the default value.
     28        This is now accomplished by having an explicit check defaultTabIndex returning 0. MathMLElement
     29        overrides defaultTabIndex so it continues to use the old logic. All this complexity should go away
     30        in webkit.org/b/199606.
     31        (WebCore::Element::setTabIndexForBindings): Renamed from setTabIndex.
     32        (WebCore::Element::isKeyboardFocusable const): Checks shouldBeIgnoredInSequentialFocusNavigation
     33        in lieu of calling Element::tabIndexForBindings.
     34        * dom/Element.h:
     35        (WebCore::Element::shouldBeIgnoredInSequentialFocusNavigation const): Added. Returns true if the
     36        old implementation of Element::tabIndex would have returned -1 due to supportsFocus returning false.
     37        * dom/ElementRareData.h:
     38        (WebCore::ElementRareData::tabIndex const): Made this function return Optional<int>. Note that
     39        ElementRareData continue to store a bit field and int for more efficient packing.
     40        * html/HTMLAnchorElement.cpp:
     41        (WebCore::HTMLAnchorElement::defaultTabIndex const): Replaced tabIndex.
     42        * html/HTMLAnchorElement.h:
     43        * html/HTMLAreaElement.cpp:
     44        (WebCore::HTMLAreaElement::isFocusable const):
     45        * html/HTMLElement.cpp:
     46        (WebCore::HTMLElement::tabIndex const): Deleted. The logic is now in Element::tabIndex itself.
     47        * html/HTMLElement.h:
     48        * html/HTMLElement.idl:
     49        * html/HTMLFormControlElement.cpp:
     50        (WebCore::HTMLFormControlElement::defaultTabIndex const): Replaced tabIndex.
     51        * html/HTMLFormControlElement.h:
     52        * mathml/MathMLElement.cpp:
     53        (WebCore::MathMLElement::defaultTabIndex const): Replaced tabIndex. This is probably a bug since
     54        this would put every MathML element in the sequential navigation order regardless of whether it
     55        has tabIndex set or not.
     56        * mathml/MathMLElement.h:
     57        * page/FocusController.cpp:
     58        (WebCore::tabIndexForElement): Added. Computes the "effective" tab index FocusController uses.
     59        (WebCore::shadowAdjustedTabIndex):
     60        (WebCore::nextElementWithGreaterTabIndex): This code should use shadowAdjustedTabIndex instead
     61        but keeping the old behavior for now.
     62        * svg/SVGAElement.cpp:
     63        (WebCore::SVGAElement::defaultTabIndex const): Replaced tabIndex.
     64        * svg/SVGAElement.h:
     65        * svg/SVGElement.cpp:
     66        (WebCore::SVGElement::tabIndex const): Deleted. The logic is now in Element::tabIndex itself.
     67        * svg/SVGElement.h:
     68        (WebCore::SVGElement::hasTagName const):
     69        * svg/SVGElement.idl:
     70
    1712019-08-16  Ross Kirsling  <ross.kirsling@sony.com>
    272

  • trunk/Source/WebCore/dom/Element.cpp

    r248669 r248784  
    244244}
    245245
    246 bool Element::tabIndexSetExplicitly() const
    247 {
    248     return hasRareData() && elementRareData()->tabIndexSetExplicitly();
     246Optional<int> Element::tabIndexSetExplicitly() const
     247{
     248    if (!hasRareData())
     249        return WTF::nullopt;
     250    return elementRareData()->tabIndex();
     251}
     252
     253int Element::defaultTabIndex() const
     254{
     255    return -1;
    249256}
    250257
    251258bool Element::supportsFocus() const
    252259{
    253     return tabIndexSetExplicitly();
     260    return !!tabIndexSetExplicitly();
    254261}
    255262
    … …  
    259266}
    260267
    261 int Element::tabIndex() const
    262 {
    263     return hasRareData() ? elementRareData()->tabIndex() : 0;
    264 }
    265 
    266 void Element::setTabIndex(int value)
     268int Element::tabIndexForBindings() const
     269{
     270    auto defaultIndex = defaultTabIndex();
     271    ASSERT(!defaultIndex || defaultIndex == -1);
     272    // FIXME: supportsFocus() check shouldn't be here.
     273    if (!defaultIndex || supportsFocus())
     274        return tabIndexSetExplicitly().valueOr(0);
     275    return defaultIndex;
     276}
     277
     278void Element::setTabIndexForBindings(int value)
    267279{
    268280    setIntegralAttribute(tabindexAttr, value);
    … …  
    271283bool Element::isKeyboardFocusable(KeyboardEvent*) const
    272284{
    273     return isFocusable() && tabIndex() >= 0;
     285    return isFocusable() && !shouldBeIgnoredInSequentialFocusNavigation() && tabIndexSetExplicitly().valueOr(0) >= 0;
    274286}
    275287

  • trunk/Source/WebCore/dom/Element.h

    r248669 r248784  
    317317    void setHasFocusWithin(bool flag);
    318318
    319     bool tabIndexSetExplicitly() const;
     319    Optional<int> tabIndexSetExplicitly() const;
     320    bool shouldBeIgnoredInSequentialFocusNavigation() const { return defaultTabIndex() < 0 && !supportsFocus(); }
    320321    virtual bool supportsFocus() const;
    321322    virtual bool isFocusable() const;
    … …  
    325326    virtual bool shouldUseInputMethod();
    326327
    327     virtual int tabIndex() const;
    328     WEBCORE_EXPORT void setTabIndex(int);
     328    virtual int tabIndexForBindings() const;
     329    WEBCORE_EXPORT void setTabIndexForBindings(int);
    329330    virtual RefPtr<Element> focusDelegate();
    330331
    … …  
    715716    ElementRareData& ensureElementRareData();
    716717
     718    virtual int defaultTabIndex() const;
     719
    717720    void detachAllAttrNodesFromElement();
    718721    void detachAttrNodeFromElementWithValue(Attr*, const AtomString& value);

  • trunk/Source/WebCore/dom/ElementRareData.h

    r243643 r248784  
    5555    void resetStyleRelations();
    5656   
    57     int tabIndex() const { return m_tabIndex; }
     57    Optional<int> tabIndex() const { return m_tabIndexWasSetExplicitly ? Optional<int> { m_tabIndex } : WTF::nullopt; }
    5858    void setTabIndexExplicitly(int index) { m_tabIndex = index; m_tabIndexWasSetExplicitly = true; }
    5959    bool tabIndexSetExplicitly() const { return m_tabIndexWasSetExplicitly; }

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

    r246490 r248784  
    331331}
    332332
    333 int HTMLAnchorElement::tabIndex() const
    334 {
    335     // Skip the supportsFocus check in HTMLElement.
    336     return Element::tabIndex();
     333int HTMLAnchorElement::defaultTabIndex() const
     334{
     335    return 0;
    337336}
    338337

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

    r246490 r248784  
    8989    bool canStartSelection() const final;
    9090    String target() const override;
    91     int tabIndex() const final;
     91    int defaultTabIndex() const final;
    9292    bool draggable() const final;
    9393

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

    r246490 r248784  
    214214        return false;
    215215
    216     return supportsFocus() && Element::tabIndex() >= 0;
     216    return supportsFocus() && tabIndexSetExplicitly().valueOr(0) >= 0;
    217217}
    218218   

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

    r246490 r248784  
    715715}
    716716
    717 int HTMLElement::tabIndex() const
    718 {
    719     if (supportsFocus())
    720         return Element::tabIndex();
    721     return -1;
    722 }
    723 
    724717bool HTMLElement::translate() const
    725718{

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

    r246490 r248784  
    4444
    4545    WEBCORE_EXPORT String title() const final;
    46 
    47     int tabIndex() const override;
    4846
    4947    WEBCORE_EXPORT ExceptionOr<void> setInnerText(const String&);

  • trunk/Source/WebCore/html/HTMLElement.idl

    r239313 r248784  
    3535    [CEReactions, Reflect] attribute boolean hidden;
    3636    void click();
    37     [CEReactions] attribute long tabIndex;
     37    [CEReactions, ImplementedAs=tabIndexForBindings] attribute long tabIndex;
    3838    void focus();
    3939    void blur();

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

    r248491 r248784  
    410410}
    411411
    412 int HTMLFormControlElement::tabIndex() const
    413 {
    414     // Skip the supportsFocus check in HTMLElement.
    415     return Element::tabIndex();
     412int HTMLFormControlElement::defaultTabIndex() const
     413{
     414    return 0;
    416415}
    417416

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

    r246490 r248784  
    168168    bool isFormControlElement() const final { return true; }
    169169
    170     int tabIndex() const final;
     170    int defaultTabIndex() const final;
    171171
    172172    bool isValidFormControlElement() const;

  • trunk/Source/WebCore/mathml/MathMLElement.cpp

    r246490 r248784  
    222222}
    223223
    224 int MathMLElement::tabIndex() const
    225 {
    226     // Skip the supportsFocus check in StyledElement.
    227     return Element::tabIndex();
     224int MathMLElement::defaultTabIndex() const
     225{
     226    // FIXME: This seems wrong.
     227    return 0;
    228228}
    229229

  • trunk/Source/WebCore/mathml/MathMLElement.h

    r246490 r248784  
    109109    bool isURLAttribute(const Attribute&) const final;
    110110    bool supportsFocus() const final;
    111     int tabIndex() const final;
     111    int defaultTabIndex() const final;
    112112};
    113113

  • trunk/Source/WebCore/page/FocusController.cpp

    r247416 r248784  
    326326}
    327327
     328// FIXME: This function should be merged into shadowAdjustedTabIndex.
     329static inline int tabIndexForElement(const Element& element)
     330{
     331    return element.shouldBeIgnoredInSequentialFocusNavigation() ? -1 : element.tabIndexSetExplicitly().valueOr(0);
     332}
     333
    328334static inline int shadowAdjustedTabIndex(Element& element, KeyboardEvent* event)
    329335{
    … …  
    332338            return 0; // Treat a shadow host without tabindex if it has tabindex=0 even though HTMLElement::tabIndex returns -1 on such an element.
    333339    }
    334     return element.tabIndex();
     340    return tabIndexForElement(element);
    335341}
    336342
    … …  
    621627            continue;
    622628        Element& candidate = downcast<Element>(*node);
    623         int candidateTabIndex = candidate.tabIndex();
     629        // FIXME: We should be calling shadowAdjustedTabIndex instead.
     630        int candidateTabIndex = tabIndexForElement(candidate);
    624631        if (isFocusableElementOrScopeOwner(candidate, event) && candidateTabIndex > tabIndex && (!winner || candidateTabIndex < winningTabIndex)) {
    625632            winner = &candidate;

  • trunk/Source/WebCore/svg/SVGAElement.cpp

    r246490 r248784  
    152152}
    153153
    154 int SVGAElement::tabIndex() const
    155 {
    156     // Skip the supportsFocus check in SVGElement.
    157     return Element::tabIndex();
     154int SVGAElement::defaultTabIndex() const
     155{
     156    return 0;
    158157}
    159158

  • trunk/Source/WebCore/svg/SVGAElement.h

    r246490 r248784  
    6161    bool isURLAttribute(const Attribute&) const final;
    6262    bool canStartSelection() const final;
    63     int tabIndex() const final;
     63    int defaultTabIndex() const final;
    6464
    6565    bool willRespondToMouseClickEvents() final;

  • trunk/Source/WebCore/svg/SVGElement.cpp

    r246490 r248784  
    182182    document().accessSVGExtensions().rebuildAllElementReferencesForTarget(*this);
    183183    document().accessSVGExtensions().removeAllElementReferencesForTarget(*this);
    184 }
    185 
    186 int SVGElement::tabIndex() const
    187 {
    188     if (supportsFocus())
    189         return Element::tabIndex();
    190     return -1;
    191184}
    192185

  • trunk/Source/WebCore/svg/SVGElement.h

    r246490 r248784  
    120120
    121121    bool hasTagName(const SVGQualifiedName& name) const { return hasLocalName(name.localName()); }
    122     int tabIndex() const override;
    123122
    124123    void callClearTarget() { clearTarget(); }

  • trunk/Source/WebCore/svg/SVGElement.idl

    r216426 r248784  
    3232    readonly attribute SVGAnimatedString className;
    3333
    34     attribute long tabIndex;
     34    [CEReactions=NotNeeded, ImplementedAs=tabIndexForBindings] attribute long tabIndex;
    3535
    3636    // FIXME: Using "undefined" as default parameter value is wrong.

  • trunk/Source/WebKit/ChangeLog

    r248783 r248784  
     12019-08-16  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Split tabIndex computation for DOM and the rest of WebCore
     4        https://bugs.webkit.org/show_bug.cgi?id=200806
     5
     6        Reviewed by Chris Dumez.
     7
     8        * WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMHTMLElement.cpp:
     9        (webkit_dom_html_element_get_tab_index):
     10        (webkit_dom_html_element_set_tab_index):
     11
    1122019-08-16  Ross Kirsling  <ross.kirsling@sony.com>
    213

  • trunk/Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMHTMLElement.cpp

    r234586 r248784  
    458458    g_return_val_if_fail(WEBKIT_DOM_IS_HTML_ELEMENT(self), 0);
    459459    WebCore::HTMLElement* item = WebKit::core(self);
    460     glong result = item->tabIndex();
     460    glong result = item->tabIndexForBindings();
    461461    return result;
    462462}
    … …  
    467467    g_return_if_fail(WEBKIT_DOM_IS_HTML_ELEMENT(self));
    468468    WebCore::HTMLElement* item = WebKit::core(self);
    469     item->setTabIndex(value);
     469    item->setTabIndexForBindings(value);
    470470}
    471471

  • trunk/Source/WebKitLegacy/mac/ChangeLog

    r248762 r248784  
     12019-08-16  Ryosuke Niwa  <rniwa@webkit.org>
     2
     3        Split tabIndex computation for DOM and the rest of WebCore
     4        https://bugs.webkit.org/show_bug.cgi?id=200806
     5
     6        Reviewed by Chris Dumez.
     7
     8        * DOM/DOMHTMLElement.mm:
     9        (-[DOMHTMLElement tabIndex]):
     10        (-[DOMHTMLElement setTabIndex:]):
     11
    1122019-08-15  Yusuke Suzuki  <ysuzuki@apple.com>
    213

  • trunk/Source/WebKitLegacy/mac/DOM/DOMHTMLElement.mm

    r247570 r248784  
    9696{
    9797    WebCore::JSMainThreadNullState state;
    98     return IMPL->tabIndex();
     98    return IMPL->tabIndexForBindings();
    9999}
    100100
    … …  
    102102{
    103103    WebCore::JSMainThreadNullState state;
    104     IMPL->setTabIndex(newTabIndex);
     104    IMPL->setTabIndexForBindings(newTabIndex);
    105105}
    106106
Note: See TracChangeset for help on using the changeset viewer.