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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cross browser assertions #1708

Merged
merged 7 commits into from
Jun 16, 2019
Merged

Fix cross browser assertions #1708

merged 7 commits into from
Jun 16, 2019

Conversation

marvinhagemeister
Copy link
Member

@marvinhagemeister marvinhagemeister commented Jun 12, 2019

This PR attempts to fix all cross-browser issues and attempts to make the CI green again. Size increase is mainly due to the IE11 workaround for select.value.

Adds +41 B 馃槓
Adds +35 B 馃檪

@marvinhagemeister marvinhagemeister force-pushed the cross_browser2 branch 2 times, most recently from d2858a0 to 4ae50c3 Compare June 12, 2019 06:14
Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some real MVP stuff here, the background things are my mistake ^^ sorry about that.

@marvinhagemeister
Copy link
Member Author

Only 1 remaining test to go 馃挭

@coveralls
Copy link

coveralls commented Jun 14, 2019

Coverage Status

Coverage increased (+0.0007%) to 99.782% when pulling 1aa984e on cross_browser2 into 3a4f568 on master.

@marvinhagemeister
Copy link
Member Author

marvinhagemeister commented Jun 15, 2019

Dug a little deeper on the IE11 select issue with a real IE11 instance. Turns out that using select.value = "A" doesn't work at all in IE11 :/

These are the other alternatives, but they all require iteration:

  • Use select.selectedIndex
  • Set options.selected

EDIT: Tried both approaches and the 2nd one is 7B smaller 馃帀

@marvinhagemeister marvinhagemeister force-pushed the cross_browser2 branch 2 times, most recently from 1df7c72 to 1f9bc90 Compare June 15, 2019 09:59
Copy link
Member

@JoviDeCroock JoviDeCroock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice find on the select bug for IE11, great work!!!

@marvinhagemeister marvinhagemeister marked this pull request as ready for review June 15, 2019 12:20
@marvinhagemeister marvinhagemeister merged commit f848a03 into master Jun 16, 2019
@marvinhagemeister marvinhagemeister deleted the cross_browser2 branch June 16, 2019 10:37
@andrewiggins
Copy link
Member

@marvinhagemeister Re: the select.value fix, what was failing? Was it the saucelabs tests? How did you test the IE11 select.value fix? I'm curious cuz I'd love to get the 35 bytes back to invest in other features (like an internal vnode object that wraps the public one).

I downloaded an IE11 Win7 VM from modern.ie and ran this jsbin in IE11 on that VM and select.value seems work.

@marvinhagemeister
Copy link
Member Author

@andrewiggins I'm happy to revert it if a smaller fix can be found 馃憤 Yup, tested it with the same VM and a real Win7 IE11 machine we have at work in our QA department.

Just checked again and the linked jsbin works for both examples. What doesn't work is when the select is pasted into the demo app. For that I used the following component and reverted this PR locally:

function App() {
	const [v, update] = useState(0);
	return (
		<div>
			<select value={v}>
				<option value="0">0</option>
				<option value="1">1</option>
				<option value="2">2</option>
				<option value="2">2</option>
				<option value="3">3</option>
			</select>
			<button onClick={() => update(Math.floor(Math.random() * 4))}>Update</button>
		</div>
	);
}

render(<App />, document.body);

Screencast of the bug:

ie11_select

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants