ember-cli-page-object icon indicating copy to clipboard operation
ember-cli-page-object copied to clipboard

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

Open sukima opened this issue 5 years ago • 11 comments

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 avatar Sep 11 '20 20:09 sukima

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 avatar Sep 11 '20 22:09 sukima

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}}

sukima avatar Sep 11 '20 22:09 sukima

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 avatar Sep 22 '20 20:09 ro0gr

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 https://github.com/san650/ember-cli-page-object/pull/466

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

ro0gr avatar Jan 20 '21 20:01 ro0gr

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?

Alonski avatar Oct 03 '21 16:10 Alonski

@Alonski would be glad to accept a PR

ro0gr avatar Oct 03 '21 16:10 ro0gr

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

Alonski avatar Oct 03 '21 17:10 Alonski

Opened a PR :)

Alonski avatar Oct 03 '21 17:10 Alonski

@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.

ro0gr avatar Oct 06 '21 06:10 ro0gr

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

Thanks!

Alonski avatar Oct 06 '21 07:10 Alonski

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 avatar Oct 13 '21 20:10 ro0gr