Skip to content

Commit

Permalink
fix: Always apply form elements value property as an attribute (#227)
Browse files Browse the repository at this point in the history
* Add failing test for inputs with already present value attributes

* Apply form elements value _property_ as an _attribute always

There's no reason for us to check to see if the value attribute exists on a DOM
node already. This was probably a check to prevent us from modifying the
customers DOM when we were mutating in place. Now we're only modifying the
clone, so it's safe to _always_ set the attribute from the DOM elements
property.

* Fix textarea integration test
  • Loading branch information
Robdel12 authored Jun 6, 2019
1 parent 7692f92 commit 8bcc318
Show file tree
Hide file tree
Showing 3 changed files with 20 additions and 10 deletions.
11 changes: 4 additions & 7 deletions src/percy-agent-client/dom.ts
Original file line number Diff line number Diff line change
Expand Up @@ -121,19 +121,16 @@ class DOM {
switch (elem.type) {
case 'checkbox':
case 'radio':
if (elem.checked && !elem.hasAttribute('checked')) {
if (elem.checked) {
cloneEl!.setAttribute('checked', '')
}
break
case 'textarea':
// setting text or value does not work but innerText does
if (elem.innerText !== elem.value) {
cloneEl!.innerText = elem.value
}
cloneEl!.innerText = elem.value
break
default:
if (!elem.getAttribute('value')) {
cloneEl!.setAttribute('value', elem.value)
}
cloneEl!.setAttribute('value', elem.value)
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion test/integration/agent-integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ describe('Integration test', () => {
const $ = cheerio.load(domSnapshot)

expect($('#testInputText').attr('value')).to.equal('test input value')
expect($('#testTextarea').attr('value')).to.equal('test textarea value')
expect($('#testTextarea').text()).to.equal('test textarea value')
expect($('#testCheckbox').attr('checked')).to.equal('checked')
expect($('#testRadioButton').attr('checked')).to.equal('checked')
})
Expand Down
17 changes: 15 additions & 2 deletions test/percy-agent-client/dom.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,9 @@ describe('DOM -', () => {
<label for="name">Name</label>
<input id="name" type="text" />
<label for="valueAttr">Already has value</label>
<input id="valueAttr" type="text" value="Already present" />
<input id="mailing" type="checkbox" />
<label for="mailing">Subscribe?</label>
Expand All @@ -180,6 +183,8 @@ describe('DOM -', () => {
`)

await type('#name', 'Bob Boberson')
// Range is to select the text and replace the current input value
await type('#valueAttr', 'Replacement Value!', { range: [0, 500]})
await type('#feedback', 'This is my feedback... And it is not very helpful')
await check('#radio')
await check('#mailing')
Expand All @@ -192,12 +197,16 @@ describe('DOM -', () => {
expect($domString('#mailing').attr('checked')).to.equal('checked')
})

it('leaves unchecked checkboxes alone', () => {
expect($domString('#nevercheckedradio').attr('checked')).to.equal(undefined)
})

it('serializes checked radio buttons', () => {
expect($domString('#radio').attr('checked')).to.equal('checked')
})

it('serializes textareas', () => {
expect($domString('#feedback').attr('value')).to.equal(
expect($domString('#feedback').text()).to.equal(
'This is my feedback... And it is not very helpful',
)
})
Expand All @@ -206,8 +215,12 @@ describe('DOM -', () => {
expect($domString('#name').attr('value')).to.equal('Bob Boberson')
})

it('serializes inputs with already present value attributes', () => {
expect($domString('#valueAttr').attr('value')).to.equal('Replacement Value!')
})

it('adds a guid data-attribute to the original DOM', () => {
expect(document.querySelectorAll('[data-percy-element-id]').length).to.equal(5)
expect(document.querySelectorAll('[data-percy-element-id]').length).to.equal(6)
})

it('adds matching guids to the orignal DOM and cloned DOM', () => {
Expand Down

0 comments on commit 8bcc318

Please sign in to comment.