pixiebrix / pixiebrix-extension

PixieBrix browser extension
https://www.pixiebrix.com
GNU Affero General Public License v3.0
83 stars 22 forks source link

Prefer id-like selectors in sorting #3321

Closed twschiller closed 2 years ago

twschiller commented 2 years ago

In sorting our inferred selectors, we should prefer id-like selectors. Not sure if this is a new bug, or didn't work previously

In the below example, the id selector should appear before the attribute selector, even though the attribute selector is shorter

image

As part of implementing, we should add a test case too

fregante commented 2 years ago

Without any info about reproducibility I can't reproduce it. What brick is it? URL?

Tested on https://pbx.vercel.app/bootstrap-5/

Screen Shot 11
fregante commented 2 years ago

https://github.com/pixiebrix/pixiebrix-extension/blob/3fbf96e9533d78af6e6ceb7956cf7b84e053ef7a/src/contentScript/nativeEditor/infer.ts#L120-L121

Used in:

https://github.com/pixiebrix/pixiebrix-extension/blob/c25888e82c8f57f937b771b92df88030b81986d8/src/contentScript/nativeEditor/infer.ts#L678-L689

Tested in:

https://github.com/pixiebrix/pixiebrix-extension/blob/3fbf96e9533d78af6e6ceb7956cf7b84e053ef7a/src/contentScript/nativeEditor/infer.test.tsx#L404-L415

fregante commented 2 years ago

While working on https://github.com/pixiebrix/pixiebrix-extension/pull/3349 I noticed that safeCssSelector is being called directly from selector.ts instead of going through inferSelector

https://github.com/pixiebrix/pixiebrix-extension/blob/eaed9609515645600fe447b1cb907aa0edf75d6f/src/contentScript/nativeEditor/selector.ts#L283

This is probably the cause of the missing sorting, but I don't know how to go through this path (selector.ts) instead of the regular one.

Something similar happens in another file, calling the library without any blacklist/whitelist:

https://github.com/pixiebrix/pixiebrix-extension/blob/eaed9609515645600fe447b1cb907aa0edf75d6f/src/contentScript/pageEditor.ts#L186-L188

Are these usages correct? Should we make further changes to avoid undesired direct usage of these functions? (e.g. configure linter, use @deprecated, rename to _fnName)

twschiller commented 2 years ago

Without any info about reproducibility I can't reproduce it. What brick is it? URL?

Apologies for the lack of information. It's the JQuery Brick on this page when you select the employee id field: https://developer.automationanywhere.com/challenges/automationanywherelabs-employeedatamigration.html

image

This is probably the cause of the missing sorting, but I don't know how to go through this path (selector.ts) instead of the regular one.

IIRC, this call is for uniquely identifying an element on the page. It's not about generating a list of human-readable selectors. In this case, I think even random CSS module names would be OK?

Something similar happens in another file, calling the library without any blacklist/whitelist:

I think this is the same situation, where we're using the selector generated to pass the selected element across contexts. The selector is never showed to the user?

EDIT: apologies for accidentally hitting close instead of comment 🤦