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.

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

STYLE_GET seems broken #546

Closed
PierreBrisorgueil opened this issue Aug 16, 2020 · 12 comments · Fixed by #570
Closed

STYLE_GET seems broken #546

PierreBrisorgueil opened this issue Aug 16, 2020 · 12 comments · Fixed by #570
Labels
area/drivers/cdp Cdp driver area/drivers HTML drivers status/review-needed type/bug Something isn't working
Milestone

Comments

@PierreBrisorgueil
Copy link
Contributor

Describe the bug
Not sure because it's my first test on it but STYLE_GET seems broken

To Reproduce

LET doc = DOCUMENT('https://news.ycombinator.com/', {
    driver: 'cdp',
    viewport: {
        width: 1920,
        height: 1080
    }
})


WAIT_ELEMENT(doc, '.storylink', 5000)

FOR el IN ELEMENTS(doc, '.title')
    RETURN STYLE_GET(el, 'font-family')

will return [{},{},{}...]

Expected behavior

font-family: Verdana, Geneva, sans-serif

Screenshots
If applicable, add screenshots to help explain your problem.

Desktop (please complete the following information):

  • Version: 0.11.1
@ziflex ziflex added status/review-needed type/bug Something isn't working area/drivers/cdp Cdp driver area/drivers HTML drivers labels Aug 16, 2020
@ziflex
Copy link
Member

ziflex commented Sep 25, 2020

Could you try it in 0.12?

@PierreBrisorgueil
Copy link
Contributor Author

PierreBrisorgueil commented Sep 26, 2020

@ziflex hello, same result on my side with ycombinator example

[{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{},{}]

example code checked quickly, seems good to me.

@d2bit
Copy link
Contributor

d2bit commented Oct 4, 2020

Hi, I've took a look and seems to me it's the correct behaviour, as it's checking for the element styles.

@ziflex what do you think about adding COMPUTED_STYLE_GET to retrieve window.getComputedStyle?
So that all applied styles to an element will be inspected.

If you say so, I'll try to implement it.

@PierreBrisorgueil
Copy link
Contributor Author

hoo, maybe you are right, I didn't even think about it, it is very rare to have properties directly on the element today, that would be a good idea 👍

@ziflex
Copy link
Member

ziflex commented Oct 5, 2020

Hi, I've took a look and seems to me it's the correct behaviour, as it's checking for the element styles.

@ziflex what do you think about adding COMPUTED_STYLE_GET to retrieve window.getComputedStyle?

So that all applied styles to an element will be inspected.

If you say so, I'll try to implement it.

Can we think about adding an optional 3rd parameter to STYLE_GET?

Also, we would need to add a new property to a HTML element.

@PierreBrisorgueil
Copy link
Contributor Author

if you add this I would advise enabling this option by default

@ziflex
Copy link
Member

ziflex commented Nov 17, 2020

I wonder what implications might be if we always return computed styles?

@davad
Copy link
Contributor

davad commented Nov 17, 2020

I think returning computed styles is the natural thing to do here. Most use-cases I can think of, you'd be interested in what styles are currently applied to an element.

The only use-case I can think for getting the style that's attached to a specific element would be UI testing (i.e. "assert that style X is attached to element Y"). Can anyone think of another use-case?

@ziflex
Copy link
Member

ziflex commented Nov 17, 2020

Ok, I think the resolution would be this:

  • STYLE_GET returns always computed values
  • el.style - returns computed values
  • el.attributes.style - returns styles defined in the element.

@PierreBrisorgueil
Copy link
Contributor Author

PierreBrisorgueil commented Nov 17, 2020

seems good!

@ziflex
Copy link
Member

ziflex commented Nov 17, 2020

@d2bit do you wanna take it?

@ziflex ziflex added this to the 0.13.0 milestone Nov 20, 2020
@PierreBrisorgueil
Copy link
Contributor Author

Thx :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/drivers/cdp Cdp driver area/drivers HTML drivers status/review-needed type/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants