Problem/Motivation
When invoking $this->getTextContent()
or $this->assertSession()->pageTextContains()
on BrowserTestBase then the plain text page content is not correct. The text content of <style>
and <script>
tags is included in the page text, but it should only contain text that a human can see on the page.
The problem is that Mink just uses PHP's DOMDocument extension to get the nodeValue from the document, which extracts the text content of all tags regardless of their meaning.
Proposed resolution
Override Mink's Session class to return our own DocumentElement when the page content is requested. That way we can filter out styles and scripts when the text version of a page is requested. This also fixes the problem for assertSession()->pageTextContains() and getTextContent() in one place and we can avoid overriding the Mink driver class.
Remaining tasks
Patch.
User interface changes
None.
API changes
None.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#28 | plain-page-2784427-28.patch | 5.01 KB | jofitz |
#28 | interdiff-25-28.txt | 1.8 KB | jofitz |
#25 | plain-page-2784427-25.patch | 4.86 KB | jofitz |
#25 | interdiff-23-25.txt | 679 bytes | jofitz |
#23 | plain-page-2784427-23.patch | 4.88 KB | jofitz |
Comments
Comment #2
klausiPatch.
Comment #3
dawehnerI'm a little bit worried with changing behaviour of mink, given that this makes potential mink documentation invalid.
On the other hand I totally agree that the page text should not contain CSS styles nor JS settings.
Is there a reason we don't just remove the elements as we go? One could also do a
array_merge($scripts, $styles)
to skip 2 of the 3 foreach loopsComment #4
klausiThe problem is that $scripts and $styles are not arrays, but traversable DOMNodeList objects. As explained in the comment DOMNodeList behaves weirdly :-(
Comment #5
dawehnerIs this actually an upstream issue we are aware of?
Ah I see, so we could use
iterator_to_array()
, as that, obviously, takes a traversable.Comment #6
klausiGood idea with iterator_to_array(), implemented that!
I don't know of any upstream issue in Mink about this yet, but it might be worth to create one.
Comment #7
dawehnerAh yeah this is indeed now looking much nicer
Comment #9
klausiRandom test fail, back to RTBC.
Comment #11
klausiRerolled, no other changes.
Comment #12
dawehnerThat's a pure bug fix, this should totally be possible for 8.2.x and 8.3.x
Comment #14
SuWagner CreditAttribution: SuWagner commentedComment #15
SuWagner CreditAttribution: SuWagner commentedComment #17
jofitz CreditAttribution: jofitz at ComputerMinds commentedRe-rolled.
Comment #19
jofitz CreditAttribution: jofitz at ComputerMinds commentedEnsure BrowserTestBase instantiates the new Drupal\Tests\Session rather than a Behat\Mink\Session.
Comment #21
klausiwe need a fully qualified name here for Session.
Otherwise looks good, thanks!
Comment #22
klausiindentation errors as reported by the testbot, use only 2 spaces instead.
Comment #23
jofitz CreditAttribution: jofitz at ComputerMinds commentedComment #24
klausiSorry, one more thing:
We don't create use statements for classes in the global namespace, see https://www.drupal.org/docs/develop/coding-standards/namespaces . Use \ directly in code for that class instead.
Comment #25
jofitz CreditAttribution: jofitz at ComputerMinds commentedYou mean the use statement you added in #2? :p
Comment #26
klausihihihi, yes, I like finding issues in my own work. Looks good now!
Comment #27
alexpottIn simpletest we strip the head content...
See #2546582: AssertContentTrait::getTextContent() includes the @import css statement, that is nuts
I don't think pageTextContains should find title text either - for example.
Comment #28
jofitz CreditAttribution: jofitz at ComputerMinds commentedRemoved the head tag within getText().
Comment #30
michielnugter CreditAttribution: michielnugter as a volunteer and at Synetic commentedComment #33
Tess BakkerComment #34
Tess BakkerComment #35
Tess BakkerDiscussed this with LenDude and we don't see the need to extend Mink in this way.
New features in Mink can break now, also it will break \Drupal\Tests\WebAssert::titleEquals()
Comment #36
Tess BakkerComment #37
dawehner@Tessa Bakker
Do I understand it correctly that you decided to keep the existing behaviour of mink? I'd be totally fine with that, especially as this means less code for us to maintain. Do you want to mark the issue as fixed?
Comment #38
Tess Bakker@dawehner, Yes, I think it would be better to leave this 'as is' and mark this issue as 'won't fix'. If a test is successful because of (inline) JS or CSS, then the test was probably wrong ;)
Comment #39
dawehnerNice!
Comment #41
alexpottThis has come up again - see #3175718: Random fails due to drupal-settings-json being counted as page text