Bug 256536

Summary: XPath: Apply ignore-case matching to attribute names
Product: WebKit Reporter: Ahmad Saleem <ahmad.saleem792>
Component: XMLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: annevk, ap, cdumez, rniwa, webkit-bug-importer
Priority: P2 Keywords: BrowserCompat, InRadar, WPTImpact
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
GitHub Patch Tried
none
Compiling version
none
Update version none

Description Ahmad Saleem 2023-05-09 11:30:17 PDT
Created attachment 466294 [details]
GitHub Patch Tried

Hi Team,

While going through Blink's commit, I came across another potential merge but I am getting some C++ errors, so not able to compile:

Blink Commit - https://chromium.googlesource.com/chromium/src.git/+/0292ac42f12b0d7725ed1a311cce46290217a3bb

WPT failing test - http://wpt.live/domxpath/002.html

WebKit Source - https://searchfox.org/wubkat/source/Source/WebCore/xml/XPathStep.cpp#189 etc.

___________

Attached screenshots does not work at Line 354 where (as per 1-1 of Blink commit):

auto attr;

^ Does not work.

_________

Appreciate if someone can guide or if someone can fix this.

Thanks!
Comment 1 Ahmad Saleem 2023-05-11 17:15:10 PDT
This is error, I get:

declaration of variable 'attr' with deduced type 'auto *' requires an
      initializer
Comment 2 Chris Dumez 2023-05-11 17:26:48 PDT
(In reply to Ahmad Saleem from comment #1)
> This is error, I get:
> 
> declaration of variable 'attr' with deduced type 'auto *' requires an
>       initializer

Probably want to use something like:
```
RefPtr<Attr> attr;

// We need this branch because getAttributeNodeNS() doesn't do
// ignore-case matching even for an HTML element in an HTML document.
if (m_nodeTest.m_namespaceURI.isNull())
    attr = contextElement->getAttributeNode(m_nodeTest.m_data);
else
    attr = contextElement->getAttributeNodeNS(m_nodeTest.m_namespaceURI, m_nodeTest.m_data);
}
```
Comment 3 Ahmad Saleem 2023-05-11 17:27:30 PDT
(In reply to Chris Dumez from comment #2)
> (In reply to Ahmad Saleem from comment #1)
> > This is error, I get:
> > 
> > declaration of variable 'attr' with deduced type 'auto *' requires an
> >       initializer
> 
> Probably want to use something like:
> ```
> RefPtr<Attr> attr;
> 
> // We need this branch because getAttributeNodeNS() doesn't do
> // ignore-case matching even for an HTML element in an HTML document.
> if (m_nodeTest.m_namespaceURI.isNull())
>     attr = contextElement->getAttributeNode(m_nodeTest.m_data);
> else
>     attr = contextElement->getAttributeNodeNS(m_nodeTest.m_namespaceURI,
> m_nodeTest.m_data);
> }
> ```

Let me try it in local build, if it works. Will do PR. Thanks for your help.
Comment 4 Ahmad Saleem 2023-05-11 17:44:15 PDT
Created attachment 466326 [details]
Compiling version

^ This attached compiles. @Chris - thanks for your help.

Although, there is no progression on WPT. Which is weird. :-(
Comment 5 Chris Dumez 2023-05-11 17:52:41 PDT
(In reply to Ahmad Saleem from comment #4)
> Created attachment 466326 [details]
> Compiling version
> 
> ^ This attached compiles. @Chris - thanks for your help.
> 
> Although, there is no progression on WPT. Which is weird. :-(

Can you try this for the top part of the change?
```
                auto& attr = downcast<Attr>(node);
                if (attr.document().isHTMLDocument() && attr.ownerElement() &&
                    attr.ownerElement()->isHTMLElement() && namespaceURI.isNull() &&
                    attr.namespaceURI().isNull()) {
                    return equalIgnoringASCIICase(attr.localName(), name);
                }
```

I think you may have converted the other part wrong from Blink also.
Comment 6 Ahmad Saleem 2023-05-11 18:12:51 PDT
Created attachment 466328 [details]
Update version

(In reply to Chris Dumez from comment #5)
> (In reply to Ahmad Saleem from comment #4)
> > Created attachment 466326 [details]
> > Compiling version
> > 
> > ^ This attached compiles. @Chris - thanks for your help.
> > 
> > Although, there is no progression on WPT. Which is weird. :-(
> 
> Can you try this for the top part of the change?
> ```
>                 auto& attr = downcast<Attr>(node);
>                 if (attr.document().isHTMLDocument() && attr.ownerElement()
> &&
>                     attr.ownerElement()->isHTMLElement() &&
> namespaceURI.isNull() &&
>                     attr.namespaceURI().isNull()) {
>                     return equalIgnoringASCIICase(attr.localName(), name);
>                 }
> ```
> 
> I think you may have converted the other part wrong from Blink also.


I tried this as well but no progression.
Comment 7 Chris Dumez 2023-05-12 08:10:42 PDT
Pull request: https://github.com/WebKit/WebKit/pull/13806
Comment 8 Chris Dumez 2023-05-12 08:11:11 PDT
(In reply to Ahmad Saleem from comment #6)
> Created attachment 466328 [details]
> Update version
> 
> (In reply to Chris Dumez from comment #5)
> > (In reply to Ahmad Saleem from comment #4)
> > > Created attachment 466326 [details]
> > > Compiling version
> > > 
> > > ^ This attached compiles. @Chris - thanks for your help.
> > > 
> > > Although, there is no progression on WPT. Which is weird. :-(
> > 
> > Can you try this for the top part of the change?
> > ```
> >                 auto& attr = downcast<Attr>(node);
> >                 if (attr.document().isHTMLDocument() && attr.ownerElement()
> > &&
> >                     attr.ownerElement()->isHTMLElement() &&
> > namespaceURI.isNull() &&
> >                     attr.namespaceURI().isNull()) {
> >                     return equalIgnoringASCIICase(attr.localName(), name);
> >                 }
> > ```
> > 
> > I think you may have converted the other part wrong from Blink also.
> 
> 
> I tried this as well but no progression.

Well, I used the code I pasted above and got a progression so I uploaded a PR. Not sure why you saw different results.
Comment 9 Ahmad Saleem 2023-05-12 08:31:36 PDT
(In reply to Chris Dumez from comment #8)
> (In reply to Ahmad Saleem from comment #6)
> > Created attachment 466328 [details]
> > Update version
> > 
> > (In reply to Chris Dumez from comment #5)
> > > (In reply to Ahmad Saleem from comment #4)
> > > > Created attachment 466326 [details]
> > > > Compiling version
> > > > 
> > > > ^ This attached compiles. @Chris - thanks for your help.
> > > > 
> > > > Although, there is no progression on WPT. Which is weird. :-(
> > > 
> > > Can you try this for the top part of the change?
> > > ```
> > >                 auto& attr = downcast<Attr>(node);
> > >                 if (attr.document().isHTMLDocument() && attr.ownerElement()
> > > &&
> > >                     attr.ownerElement()->isHTMLElement() &&
> > > namespaceURI.isNull() &&
> > >                     attr.namespaceURI().isNull()) {
> > >                     return equalIgnoringASCIICase(attr.localName(), name);
> > >                 }
> > > ```
> > > 
> > > I think you may have converted the other part wrong from Blink also.
> > 
> > 
> > I tried this as well but no progression.
> 
> Well, I used the code I pasted above and got a progression so I uploaded a
> PR. Not sure why you saw different results.

Win is a win.. More failing cases fixed on WPT.. :-)
Comment 10 EWS 2023-05-12 15:28:49 PDT
Committed 264030@main (0386e818321f): <https://commits.webkit.org/264030@main>

Reviewed commits have been landed. Closing PR #13806 and removing active labels.
Comment 11 Radar WebKit Bug Importer 2023-05-12 15:29:26 PDT
<rdar://problem/109283215>