I'm planning to make XULElement getAttribute return null with non-existent attributes instead of an empty string

[Brian Grinstead]

Right now if you do `xulEl.getAttribute(“foo")` and the attribute is missing it will return `""`. With `htmlEl.getAttribute(“foo")` it will return null. I’d like to make the xul element behavior consistent with HTMLElement.

There's frontend code / tests that rely on the current behavior, so rather than having to deal with this one by one as we migrate XUL elements to HTML I think we should have an up-front change where we change those callers and update the behavior.

It seems tricky to find every caller that would have it's behavior change from this, but the things I expect to come up are test assertions, and stuff like:

```
// String concatenation
xulEl.getAttribute("foo") + "string” // “string”
htmlEl.getAttribute("foo") + "string" // "nullstring"

// Expecting it to always return a string which will throw once it’s null:
docTitle = tab.getAttribute("label").replace(/\0/g, "”);
```

I’m hoping our tests will catch most issues like this.

Thanks,
Brian

_______________________________________________
firefox-dev mailing list
firef...@mozilla.org
https://mail.mozilla.org/listinfo/firefox-dev

[Mike Taylor]

On 11/6/19 3:33 PM, Brian Grinstead wrote:
> Right now if you do `xulEl.getAttribute(“foo")` and the attribute is missing it will return `""`. With `htmlEl.getAttribute(“foo")` it will return null. I’d like to make the xul element behavior consistent with HTMLElement.
>
> There's frontend code / tests that rely on the current behavior, so rather than having to deal with this one by one as we migrate XUL elements to HTML I think we should have an up-front change where we change those callers and update the behavior.
>
> It seems tricky to find every caller that would have it's behavior change from this, but the things I expect to come up are test assertions, and stuff like:
>
> ```
> // String concatenation
> xulEl.getAttribute("foo") + "string” // “string”
> htmlEl.getAttribute("foo") + "string" // "nullstring"
>
> // Expecting it to always return a string which will throw once it’s null:
> docTitle = tab.getAttribute("label").replace(/\0/g, "”);
> ```
>
> I’m hoping our tests will catch most issues like this.

I have some (4 years old...) abandoned patches in Bug 232598 that may or
may not be useful here. Tests will definitely complain at you -- the
bulk of the work was fixing tests.

(I'm not really sure why I gave up, tbh.)

--
Mike Taylor
Web Compat, Mozilla

[Matthew N.]

This sounds like a great idea because I actually don't think I knew about this. It seems like a difference from HTML that could easily introduce bugs and therefore would be nice to remove.

[Brian Grinstead]

Thanks for the heads up Mike! I guess I should give precedence to the 16 year old bug and dupe my brand new one to that :).

Brian

[Gijs Kruitbosch]

On 06/11/2019 21:33, Brian Grinstead wrote:
> It seems tricky to find every caller that would have its behavior change from this

You could probably come up with an eslint rule to work out when a
<something>.getAttribute() call was used as one input to an operator
where null vs. "" as a return value would make a difference? So you'd
ignore cases where we `||` or `&&` the value, but not cases where we
access the value's properties or concatenate it or whatever. For
comparisons (===, ==, !=, !==), you could ignore everything except
comparisons with null and empty string.

That last case will be rare given our general coding style preference
for checking falsy-ness over comparisons with null/emptystring, e.g. for
!=/== "" or null - searchfox (incomplete of course because of multiline
statements etc.) finds only:

https://searchfox.org/mozilla-central/search?q=%5C.getAttribute%5C(%5B%5E%5C)%5D*%5C)+!%3F%3D%3D%3F+%22%22&case=false&regexp=true&path=.js
https://searchfox.org/mozilla-central/search?q=%5C.getAttribute%5C(%5B%5E%5C)%5D*%5C)+!%3F%3D%3D%3F+null&case=false&regexp=true&path=.js

where I think all of the `null` results will still be correct, and of
the comparisons with "", only the sortDirection one needs fixing (the
video controls one checks `hasAttribute` on the previous line so is
actually looking for empty attributes).

This would leave false negatives where we assign the value into a
variable and use it later - which sadly seems like the much more common
case, where automated analysis won't help much unless we have actual
dataflow analysis where we can work out how the resulting expression is
used... and even then, in some cases we'll "know" that the element in
question has the attribute, so there's no point worrying about the case
where it doesn't. :-(

~ Gijs

[Philipp Kewisch]

Hey Folks,

too bad we don't support optional chaining yet, this might have made the
transition a bit easier:

> tab.getAttribute("label")?.replace(/\0/g, "”);

Would it make sense to prioritize optional chaining before making this
change or would that conflict with your followup changes' timing?

https://bugzilla.mozilla.org/show_bug.cgi?id=1566143

Philipp

> On 7. Nov 2019, at 9:59 PM, Brian Grinstead <bgrin...@mozilla.com>
> wrote:
>
> Thanks for the heads up Mike! I guess I should give precedence to the