Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Implement set-attr scriptlet #147

Closed
Alex-302 opened this issue Sep 20, 2021 · 12 comments
Closed

Implement set-attr scriptlet #147

Alex-302 opened this issue Sep 20, 2021 · 12 comments
Assignees
@stanislav-atr
Labels
enhancement Improvement of existent feature Resolution: Done Status: Closed Version: Scriptlets v1.5
Milestone
1.5

Comments

@Alex-302
Copy link
Member

https://github.com/AdguardTeam/Scriptlets/blob/master/wiki/about-scriptlets.md#-%EF%B8%8F-remove-attr
For example here when data-fc attribute is removed, download is not working. But works, when removed value of attribute. It seems oprional parameter can be added - 1 remove attr, 0 remove value only.
image
@Alex-302 Alex-302 added the enhancement Improvement of existent feature label Sep 20, 2021
@ameshkov
Copy link
Member

Maybe let's consider set-attr instead? It could work the same way as the set-constant scriptlet.

@Alex-302
Copy link
Member Author

Indeed. Thought it might be unsafe.

@ameshkov
Copy link
Member

With a limited set of values it should be okay.

@slavaleleka slavaleleka added this to the 1.5 milestone Sep 27, 2021
@slavaleleka
Copy link
Contributor

so let's start with only one predefined value — empty string '', shall we?
the list of values will be extended in future when we need it

@slavaleleka slavaleleka changed the title Improve remove-attr scriptlet Implement set-attr scriptlet Sep 30, 2021
@ameshkov
Copy link
Member

@slavaleleka yup, sounds reasonable. @AdguardTeam/filters-maintainers are there any rules where other values might be needed?

@AdamWr
Copy link
Member

AdamWr commented Sep 30, 2021

If I understand correctly, it sounds like one of the points from this issue - #106

So maybe adding numbers will be useful.

@Alex-302
Copy link
Member Author

And what about agtrusted- #106 (comment) with no limits?

@ameshkov
Copy link
Member

ameshkov commented Sep 30, 2021 •
edited
Loading

@AdamWr @Alex-302 tbh I don't see why we cannot allow setting arbitrary height or width for an element. This is not the same as allowing an arbitrary string.

For instance, we could allow doing something like this: set-attr('height', '10px') and allow any dimension there (px, em, anything else?). In this case, there's no need to make this scriptlet "trusted".

@AdamWr
Copy link
Member

AdamWr commented Sep 30, 2021 •
edited
Loading

Okay.
By the way, if I'm not wrong, height attribute doesn't need px (it's added by default) and it looks like that em doesn't work at all (it's changed to px), but % seems to be supported.

@ameshkov
Copy link
Member

@AdamWr yep, you're right, it only supports px or % (or no dimension which is the same as px).

@slavaleleka slavaleleka self-assigned this Oct 1, 2021
@stanislav-atr
Copy link
Contributor

stanislav-atr commented Nov 29, 2021 •
edited
Loading

example.org#%#//scriptlet('set-attr', 'selector', 'attr',[ value])

  • selector - required, CSS selector, specifies DOM nodes to set attributes on.
  • attrs - required, a string, attribute to be set.
  • value - optional, the value to assign to the attribute, defaults to ''.
    • '' - empty string,
    • whole positive number - should be <= 32767, for the ability to set height and width attributes.

@AdamWr, percent is supported by height/width attributes but is only viable if the element's parent has correlated attribute set explicitly (in px), therefore i don't see much sense in supporting % here. Also:

If you want to make the div height a percentage of the viewport height, every ancestor of the div, including <html> and <body>, have to have height: 100%, so there is a chain of explicit percentage heights down to the div.

We may also make value an optional argument, making '' the default parameter.

@ameshkov, @slavaleleka

@ameshkov
Copy link
Member

ameshkov commented Dec 2, 2021

@stanislav-atr works for me, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@stanislav-atr stanislav-atr

Labels
enhancement Improvement of existent feature Resolution: Done Status: Closed Version: Scriptlets v1.5
Projects
None yet
Milestone
1.5
Development

No branches or pull requests
6 participants
@ameshkov @Alex-302 @AdamWr @adguard-bot @slavaleleka @stanislav-atr