WebKit / Speedometer

An open source repository for the Speedometer benchmark
Other
592 stars 70 forks source link

Use real id attribute instead of data-id? #341

Closed rniwa closed 6 months ago

rniwa commented 8 months ago

I've noticed that a bunch of test cases use data-id attribute instead of the actual id attribute. This ends up disabling / working around optimizations many browser engines have for id attribute but is this really a common practice? I'd imagine most developers would use the regular id attribute, wouldn't they?

bgrins commented 8 months ago

@julienw do you remember if there's a reason for these? https://github.com/search?q=repo%3AWebKit%2FSpeedometer%20data-id&type=code. The suspicion from the meeting is that this was coincidentally done at some point and got copy pasted to a couple other tests.

flashdesignory commented 8 months ago

quick search result: looks like it's been carried over from 2.1 examples. For example the jQuery version used it already in the commit "Add Speedometer 2.1 content."

https://github.com/WebKit/Speedometer/commit/a1e842164bc1803be6afa658bbb1336ecc726b12#diff-9744172a4fab9331d3f08502c2561c2c09d27907550fc52852a072738446abcdR30

or direct link: https://github.com/WebKit/Speedometer/blob/a1e842164bc1803be6afa658bbb1336ecc726b12/resources/todomvc/architecture-examples/jquery/index.html#L30

julienw commented 8 months ago

Web developers have been instructed for a long time to NOT use ID because they bring some issues, most notably they need to be unique across the full document, when used in a CSS selector their specificity skyrockets, they also have constraints for the characters (for example it needs to start with a letter... which is an issue for ids, right?).

I can see folks coding like it is here, and others doing string concatenation with IDs or classes. The use of data attributes is common, as is the use of attributes in selectors.

I did a unscientific survey by opening a few high-profile websites and using the devtools to search for querySelector in all JS files, and skimmed through the results. I barely see querySelector with ids (but I didn't look at getElementById which is probably more used in this case), I've seen mostly classes-based selectors, but also a few attribute-based selectors, and some pseudo-classes too (:hover, :checked, things like that).

Therefore I'd keep these selectors because the attribute selector use case is used by developers.

flashdesignory commented 8 months ago

Another use-case is testing. I've seen data-testid and data-automationid in large sites.

bgrins commented 8 months ago

Perhaps it could be renamed to data-todoid or data-todoindex or similar on relevant tests to remove some ambiguity?

rniwa commented 8 months ago

Web developers have been instructed for a long time to NOT use ID because they bring some issues, most notably they need to be unique across the full document, when used in a CSS selector their specificity skyrockets, they also have constraints for the characters (for example it needs to start with a letter... which is an issue for ids, right?).

I can see folks coding like it is here, and others doing string concatenation with IDs or classes. The use of data attributes is common, as is the use of attributes in selectors.

This is the first I've heard of such a claim. Where can we locate the data indicating this is actually what's happening in the real world? As far as I checked, popular websites / web apps like Gmail, Facebook, GitHub, etc... all use getElementById.

julienw commented 8 months ago

I should have used more nuance, sorry about that. I'm not saying getElementById is not used, I'm sure it is. The advise against using IDs is especially for styling with CSS, not for JS interaction.

I wanted to highlight that when I looked on some websites I saw uses of the attribute selector in querySelector.

I looked at github just know, here is the markup for an issue row in the issue list:

<div id="issue_4468" class="Box-row Box-row--focus-gray p-0 mt-0 js-navigation-item js-issue-row navigation-focus" data-id="1581389889" data-pjax="#repo-content-pjax-container" data-turbo-frame="repo-content-turbo-frame">

So we see they use both an id (concatenation of some text and the issue number) and a data-id (I dont know what this ID is). I have no idea what they do with that later on though, despite I instrumented querySelector, querySelectorAll and getElementById, I couldn't see calls looking like this.

The instrumentation showed all kinds of calls to qS, qSA (with mixes of classes, attibutes, elements) and getElementById. It's safe to assume that everything is used.

rniwa commented 8 months ago

I should have used more nuance, sorry about that. I'm not saying getElementById is not used, I'm sure it is. The advise against using IDs is especially for styling with CSS, not for JS interaction.

I wanted to highlight that when I looked on some websites I saw uses of the attribute selector in querySelector.

I looked at github just know, here is the markup for an issue row in the issue list:

<div id="issue_4468" class="Box-row Box-row--focus-gray p-0 mt-0 js-navigation-item js-issue-row navigation-focus" data-id="1581389889" data-pjax="#repo-content-pjax-container" data-turbo-frame="repo-content-turbo-frame">

So we see they use both an id (concatenation of some text and the issue number) and a data-id (I dont know what this ID is). I have no idea what they do with that later on though, despite I instrumented querySelector, querySelectorAll and getElementById, I couldn't see calls looking like this.

The instrumentation showed all kinds of calls to qS, qSA (with mixes of classes, attibutes, elements) and getElementById. It's safe to assume that everything is used.

I'm sure querySelector and querySelectorAll are used with all sorts of values. The problem I have with usage here is that these data-id are used to uniquely identify an element. I don't think that's a common usage. But the fact benchmark has this construct will encourage browser engines to implement optimizations for this specific usage pattern (attributes that behave like IDs to uniquely identify an element).

rniwa commented 6 months ago

Looks like we're not doing this at least for v3 at this point, closing.