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

fix(driver): fix firefox moveToStart,moveToEnd, add tests #7970

Closed
wants to merge 13 commits into from

Conversation

sainthkh
Copy link
Contributor

@sainthkh sainthkh commented Jul 14, 2020

I had to use collapse functions. They work in all modern browsers.

User facing changelog

Added tests for undocumented features, {moveToStart} and {moveToEnd}.

Additional details

  • Why was this change necessary? To reveal these features to the users.
  • What is affected by this change? N/A
  • Any implementation details to explain? N/A

How has the user experience changed?

N/A

PR Tasks

@cypress-bot
Copy link
Contributor

cypress-bot bot commented Jul 14, 2020

Thanks for taking the time to open a PR!

@sainthkh sainthkh self-assigned this Jul 14, 2020
@sainthkh
Copy link
Contributor Author

sainthkh commented Jul 14, 2020

The cause of failure in #6203 was invoke() call to contenteditable. It makes Firefox handle it as height===0. Fixed it by removing invoke.

@sainthkh sainthkh marked this pull request as ready for review July 14, 2020 03:04
@sainthkh sainthkh requested a review from kuceb July 14, 2020 03:04
@jennifer-shehane jennifer-shehane self-requested a review July 16, 2020 05:42
Copy link
Member

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

I think this test should go into the type_special_chars_spec.js file. I know our type spec was getting out of hand and we were trying to split some things out.

I'm planning to open a docs PR to document this here: cypress-io/cypress-documentation#2991

I'd like to understand a bit more about how this is different from end and home chars. It seems that this test functions exactly the same when replaced with home and end. If there is a difference in behavior from home and end, I'd like if that were covered in the test.

As far as I can tell though, I think the only difference is the association with the actual home and end key issuing the which, key and keyCode, which moveToStart and moveToEnd wouldn't do. Can I get some clarification on this?

@sainthkh
Copy link
Contributor Author

I put it in type_spec because they're more of special features than characters.

But they're {ctrl}{home}, {ctrl}{end} in most editors. In that viewpoint, they're special characters.

I'll move them to type_special_chars_spec.js.


When the text is a single line like <input>, there's no difference. But when it becomes multi-line like <textarea> or contenteditable, there's difference.

Let's say there is a text like this in <textarea> and the cursor is at the end of the text.

Cypress
is amazing.

With {home} and 123, it's:

Cypress
123is amazing.

With {moveToStart} and 123, it's:

123Cypress
is amazing.

When I was migrating the test, I thought the tests reflect this difference. But I read it again now, and it didn't.

I'll fix the test.

@jennifer-shehane
Copy link
Member

jennifer-shehane commented Jul 17, 2020

@sainthkh Awesome, yah I wanted more clarification for docs as well. This is helpful and the modified test to test this would be great.

Yah, I was just thinking the other day about special chars, how we've kind of mixed in special functions into this? Like selectAll and moveToEnd. I may split these up in the docs to be clearer that they don't correspond to a single character key. cypress-io/cypress-documentation#2991

@sainthkh
Copy link
Contributor Author

sainthkh commented Jul 17, 2020

@bkucera @jennifer-shehane

After trying to write tests that show the difference between {home} and {moveToStart} / {end} and {moveToEnd}, I found that they're buggy with contenteditable.

I had to fix them, too.

The cause was selection.modify(..., 'line') below:

if ($elements.isContentEditable(el)) {
$elements.callNativeMethod(doc, 'execCommand', 'selectAll', false, null)
const selection = doc.getSelection()
if (!selection) {
return
}
// collapsing the range doesn't work on input/textareas, since the range contains more than the input element
// However, IE can always* set selection range, so only modern browsers (with the selection API) will need this
const direction = toStart ? 'backward' : 'forward'
selection.modify('move', direction, 'line')
return
}

It's because it doesn't select to the start or the end when the text becomes long. You can check this behavior at the example here.

I had to use collapse functions. They work in all modern browsers.

And I also met a bug(?) in Firefox. (I cannot say for sure here because these selection APIs are experimental.) I bypassed it, too.

@sainthkh sainthkh marked this pull request as draft July 17, 2020 09:37
@sainthkh sainthkh force-pushed the issue-5502 branch 2 times, most recently from 0e4ed47 to bfea619 Compare July 22, 2020 02:38
@sainthkh sainthkh marked this pull request as ready for review July 22, 2020 08:29
@sainthkh
Copy link
Contributor Author

sainthkh commented Jul 22, 2020

Flaky failure.

Coding is done. I'm now trying to write some notes for the reviewer about why the change was necessary and what are changed.

All of us thought this PR would be done by adding some tests. But the problem was much bigger than that.

@sainthkh sainthkh marked this pull request as draft July 22, 2020 08:32
@sainthkh
Copy link
Contributor Author

Besides the bug caused by line I mentioned above, there were a lot of difference between Chrome and FireFox.

  • FireFox doesn't empty the content editable when it's <div><br></div>. It had to be emptied manually.
  • FireFox appends text after the content directly. So, I had to select the last element in the content directly. But there are bare text starting or ending inside, I had to select the entire content editable div.

@sainthkh sainthkh marked this pull request as ready for review July 23, 2020 07:52
@jennifer-shehane jennifer-shehane dismissed their stale review July 23, 2020 08:35

Dismissing my previous review for more thorough code reviews.

@jennifer-shehane jennifer-shehane removed their request for review July 23, 2020 08:35
@kuceb
Copy link
Contributor

kuceb commented Aug 27, 2020

@sainthkh sorry for taking so long to get to this. Let me know your thoughts on my most recent changes. Basically kept the old code but replaced selection.modify(...) with collapseTo{Start,End}

I think we shouldn't try to handle the firefox newline oddity, since it actually happens if you ctrl+a leftarrow manually.

@jennifer-shehane jennifer-shehane changed the title Add tests for {moveToStart}, {moveToEnd} chore: Add tests for {moveToStart}, {moveToEnd} Aug 28, 2020
@sainthkh
Copy link
Contributor Author

sainthkh commented Aug 28, 2020

I tried it on this editor on firefox. But there was no newline between def and 123.

Peek 2020-08-28 15-55

I opened dom.html and tested it and there was no newline between def and 123.

Peek 2020-08-28 15-53

And does this happen when you use ctrl+home and ctrl+end?

@kuceb
Copy link
Contributor

kuceb commented Aug 28, 2020

@sainthkh those are textareas, I'm referring to contenteditables (divs with contenteditable attr)

@kuceb
Copy link
Contributor

kuceb commented Aug 28, 2020

@sainthkh ok, I realize we do need to find a way to avoid firefox not removing the newline when we moveToStart/End. Looking into how to do that without having to mutate innerText, might be able to just fix the selection to be inside the proper node

@sainthkh
Copy link
Contributor Author

We cannot use line option of the selection.modify here. Because it only expands the selection by a "line", not the entire document inside contenteditable div. That's why the test result is 123\n456\ndef789\nabcghi where def is just a line above the last line.

So, I used documentboundary option.


// FireFox doesn't clear empty <br> in the content editable.
if (Cypress.browser.family === 'firefox' && root.innerText === '\n') {
  root.innerText = ''
}

The code above was necessary to solve the failures in driver firefox tests.

@kuceb kuceb changed the title chore: Add tests for {moveToStart}, {moveToEnd} fix(driver): fix firefox moveToStart,moveToEnd, add tests Aug 31, 2020
@kuceb
Copy link
Contributor

kuceb commented Sep 1, 2020

@sainthkh think I found a solution for the firefox newline issues, I moved this PR to an internal branch to speed up CI #8466 :)

@kuceb kuceb closed this Sep 1, 2020
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.

add tests for {moveToStart} / {moveToEnd} special sequences
3 participants