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

content with display: none is still captured (does not match innerText) #514

Open
sukima opened this issue Sep 11, 2020 · 11 comments · Fixed by #547
Open

content with display: none is still captured (does not match innerText) #514

sukima opened this issue Sep 11, 2020 · 11 comments · Fixed by #547
Labels

Comments

@sukima
Copy link
Contributor

sukima commented Sep 11, 2020

The PageObject.text helper returns hidden text. While — by comparison — innerText does not.

Given

<span id="example">
  <span>foo</span>
  <span style="display: none">bar</span>
  <span>baz</span>
</span>
const page = PageObject.create({
  example: PageObject.text('#example')
});

Expected

find('#example').innerText.trim() // => "foo baz"
page.example                      // => "foo baz"

Actual

find('#example').innerText.trim() // => "foo baz"
page.example                      // => "foo bar baz"
@sukima
Copy link
Contributor Author

sukima commented Sep 11, 2020

This is how I am getting around this currently.

const page = create({
  example: getter(function() {
    let [element] = findOne(this, '#example');
    return element.innerText.trim();
  })
});

@sukima
Copy link
Contributor Author

sukima commented Sep 11, 2020

And just in case someone wonders why this is an issue here is an example use case:

With this CSS

.auto-hiding-delimeter:first-child,
.auto-hiding-delimeter:last-child,
.auto-hiding-delimeter + .auto-hiding-delimeter {
  display: none;
}

You can do things like these

{{#each @things as |thing|}}
  {{#if thing}}<span class="thing">{{thing}}</span>{{/if}}
  <span class="auto-hiding-delimiter">[seperator]</span>
{{/each}}
{{#if this.boolFlagOne}}<span>1</span>{{/if}}
<span class="auto-hiding-delimiter">|</span>
{{#if this.boolFlagTwo}}<span>2</span>{{/if}}
<span class="auto-hiding-delimiter">|</span>
{{#if this.boolFlagThree}}<span>3</span>{{/if}}
<span class="auto-hiding-delimiter">|</span>
{{#if this.boolFlagFour}}<span>4</span>{{/if}}
<span class="auto-hiding-delimiter">|</span>
{{#if this.boolFlagFive}}<span>5</span>{{/if}}

@ro0gr
Copy link
Collaborator

ro0gr commented Sep 22, 2020

Thanks for reporting!

This is surprising to me. I didn't know that jquery uses textContent for the text( https://github.com/jquery/jquery/blob/e35fb62db4fb46f031056bb53e393982c03972a1/src/core.js#L276.

I think replacing our current usage of the $.text( with the innerText would be a breaking change, even though it can be treated as a fixing breaking change.. So I tend to avoid it in v1.

A side note: due to an intention to enable node.js support, we'd probably need to distinct between innerText for DOM, and textContent for no-DOM(probably via adapters).

@ro0gr
Copy link
Collaborator

ro0gr commented Jan 20, 2021

oh.. I've just faced the issue when upgraded ec-page-object in a lib from 1.16 to 1.17. This seems to be a regression introduced in #466

I believe we should revert contains to keep using $.text() instead of .innerText

@ro0gr ro0gr added the bug label Jan 20, 2021
@Alonski
Copy link
Contributor

Alonski commented Oct 3, 2021

So I ran into this today as well :)
Have an element that is visibility: hidden but I want to check if it contains text 🙃

Any plans to revert this breaking change?

@ro0gr
Copy link
Collaborator

ro0gr commented Oct 3, 2021

@Alonski would be glad to accept a PR

@Alonski
Copy link
Contributor

Alonski commented Oct 3, 2021

What would you recommend the fix to be?
I created a helper function in my project that filters over text.contains.

Alonski added a commit to Alonski/ember-cli-page-object that referenced this issue Oct 3, 2021
The change done in this PR: san650#466 caused a regression.

$.text() can find the text in `visibility: hidden` or `display: none` elements.

element.innerText does not return the text. 

Fixes: san650#514
@Alonski
Copy link
Contributor

Alonski commented Oct 3, 2021

Opened a PR :)

@ro0gr
Copy link
Collaborator

ro0gr commented Oct 6, 2021

@Alonski sorry for the late reply.

Yes, I believe, basically, we just need to drop innerText in favour of jquery's .text() method for now.

A thing I'm worried about at this point, is that a fix may appear a breaking change for users who was to update their test suites according to this innerText regression. Now they would be forced to revert their adjustments again. However, I approach it as a bugfix. So if we'd deliver a fix, I apologize in advance if it causes an issue for users.

@Alonski
Copy link
Contributor

Alonski commented Oct 6, 2021

Thanks @ro0gr
I opened a PR. If you think it's good I'd love for it to get merged and released.

Thanks!

@ro0gr
Copy link
Collaborator

ro0gr commented Oct 13, 2021

so seems I've confused 2 different issues here. At some point I've faced a regression with the contains(, while this issue is about the text(, which didn't have a regression, and it worked this way for a long time. Sorry for that! 😵‍💫

I think this may be a good feature to think about in general, but I'm reluctant to break the existing text( behavior at least in scope of v1.

@ro0gr ro0gr added feature and removed bug labels Oct 13, 2021
@ro0gr ro0gr reopened this Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants