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.

Sign up for GitHub

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

Jump to bottom

Fix: make meta+A behaviour of selecting all blocks work on safari and firefox #8180

Merged
merged 1 commit into from
Nov 1, 2018
Merged

Fix: make meta+A behaviour of selecting all blocks work on safari and firefox #8180

merged 1 commit into from
Nov 1, 2018

Conversation

jorgefilipecosta
Copy link
Member

@jorgefilipecosta jorgefilipecosta commented Jul 24, 2018 •
edited
Loading

On safari and firefox isEntirelySelected( target ) called right after meta+A event is fired when meta key is not released in the middle returns false but the content gets selected anyway.
If the target is editable it is safe to assume TinyMCE will make sure all content gets selected so in this cases we set the value to true without calling isEntirelySelected.

Fixes: #7445

How has this been tested?

I added some paragraphs.
On safari and firefox, I checked that when we press meta+A in paragraph two times all the blocks get selected.

@jorgefilipecosta jorgefilipecosta added the [Type] Bug An existing feature does not function as intended label Jul 24, 2018
@jorgefilipecosta jorgefilipecosta mentioned this pull request Jul 25, 2018
Closed
@jorgefilipecosta jorgefilipecosta force-pushed the fix/meta-A-select-all-blocks-accross-browsers branch from 5a320d4 to 4503511 Compare August 1, 2018 16:25
@jorgefilipecosta jorgefilipecosta requested a review from a team August 17, 2018 19:24
@jorgefilipecosta jorgefilipecosta force-pushed the fix/meta-A-select-all-blocks-accross-browsers branch from 4503511 to 0372e34 Compare August 17, 2018 19:29
noisysocks
noisysocks approved these changes Aug 20, 2018
View reviewed changes
Copy link
Member

@noisysocks noisysocks left a comment

Choose a reason for hiding this comment

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

I've tested this in Chrome, Firefox, Safari, Edge, and IE 11 and the bug is fixed 👍

Might be good to get another pair of eyes on this since I'm not 100% on how it works.

packages/editor/src/components/writing-flow/index.js Outdated
// but on some browsers (safari and firefox) calling isEntirelySelected right way still returns false.
// So to make sure we are compatible with this browsers we set is entirely selected to true
// after the first meta+a keypress assuming TinyMCE will make all content entirely selected.
this.isEntirelySelected = target.isContentEditable ? true : isEntirelySelected( target );
Copy link
Member

Choose a reason for hiding this comment

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

I'm trying to wrap my head around this logic. When would primary+a not result in all of the text being selected? In other words, why can't this line be:

this.isEntirelySelected = true;

Copy link
Member

Choose a reason for hiding this comment

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

I'm very hesitant of anything which forks logic based on browser differences. Can we simplify to what @noisysocks said? Should that live in isEntirelySelected ? Do we need isEntirelySelected at all? What is it about being isContentEditable that makes us assume it's entirely selected?

aduth
aduth previously requested changes Aug 20, 2018
View reviewed changes
packages/editor/src/components/writing-flow/index.js Outdated
// but on some browsers (safari and firefox) calling isEntirelySelected right way still returns false.
// So to make sure we are compatible with this browsers we set is entirely selected to true
// after the first meta+a keypress assuming TinyMCE will make all content entirely selected.
this.isEntirelySelected = target.isContentEditable ? true : isEntirelySelected( target );
Copy link
Member

Choose a reason for hiding this comment

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

I'm very hesitant of anything which forks logic based on browser differences. Can we simplify to what @noisysocks said? Should that live in isEntirelySelected ? Do we need isEntirelySelected at all? What is it about being isContentEditable that makes us assume it's entirely selected?

@jorgefilipecosta jorgefilipecosta force-pushed the fix/meta-A-select-all-blocks-accross-browsers branch from 0372e34 to 5e4d8dc Compare August 22, 2018 19:53
@jorgefilipecosta
Copy link
Member Author

jorgefilipecosta commented Aug 22, 2018 •
edited
Loading

Hi @aduth, @noisysocks I checked that in fact, we could simplify the logic to what @noisysocks said.
The isEntirelySelected function returns true if it is called on a non-content editable. Given that for contenEidtables we know that when we press primary + A the content gets selected I think we can safely simplify the logic to just set the flag to true after primary + A is pressed.

@mcsf mcsf mentioned this pull request Oct 15, 2018
Merged
@designsimply designsimply added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... labels Oct 30, 2018
@designsimply designsimply mentioned this pull request Oct 30, 2018
Closed
@jorgefilipecosta jorgefilipecosta dismissed aduth’s stale review November 1, 2018 11:43

I think the feedback was addressed and the code was simplified to the solution proposed by @noisysocks.

@jorgefilipecosta
Fix: make meta+A behaviour of selecting all blocks work on safari and…
56819c6 
… firefox.

On safari and firefox isEntirelySelected( target ) called right after meta+A event is fired without releasing the meta key in the middle returns false but content gets selected anyway. If the target is editable it is safe to assume TinyMCE will make sure all content gets selected so in this cases we set the value to true without calling isEntirelySelected.
@jorgefilipecosta jorgefilipecosta force-pushed the fix/meta-A-select-all-blocks-accross-browsers branch from 5e4d8dc to 56819c6 Compare November 1, 2018 11:50
@jorgefilipecosta jorgefilipecosta added this to the 4.3 milestone Nov 1, 2018
@jorgefilipecosta jorgefilipecosta merged commit b3e0e89 into master Nov 1, 2018
@jorgefilipecosta jorgefilipecosta deleted the fix/meta-A-select-all-blocks-accross-browsers branch November 1, 2018 13:06
daniloercoli added a commit that referenced this pull request Nov 1, 2018
@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
4e7dd62 
…rnmobile/port-quote-block-step-1

* 'master' of https://github.com/WordPress/gutenberg: (22 commits)
  Add removed periods to block descriptions. (#11367)
  Remove findDOMNode usage from the inserter (#11363)
  Remove deprecated componentWillReceiveProps from TinyMCE component (#11368)
  Create file blocks when dropping multiple files at once (#11297)
  Try avoiding the deprecated findDOMNode API from DropZone Provider (#11168)
  Fix: make meta+A behaviour of selecting all blocks work on safari and firefox. (#8180)
  Remove _wpGutenbergCodeEditorSettings and wp.codeEditor assets (#11342)
  Remove the Cloudflare warning (#11350)
  Image Block: Use source_url for media file link (#11254)
  Enhance styling of nextpage block using the Hr element (#11354)
  Embed block refactor and tidy (#10958)
  Nonce Middleware: Wrap the nonce middleware function into it's own function that isn't regenerated on every API request. (#11347)
  Fix RTL block alignments (#11293)
  RichText: fix buggy enter/delete behaviour (#11287)
  Remove code coverage setup (#11198)
  Parser: Runs all parser implementations against the same tests (#11320)
  Stop trying to autosave when title and classic block content both are empty. (#10404)
  Fix "Mac OS" typo + use fancy quotes consistently (#11310)
  Update documentation link paths (#11324)
  Editor: Reshape blocks state under own key (#11315)
  ...

# Conflicts:
#	gutenberg-mobile
@designsimply designsimply mentioned this pull request Nov 27, 2018
Closed
@designsimply designsimply added the [Feature] Block Multi Selection The ability to select and manipulate multiple blocks label Jan 8, 2019
@designsimply designsimply mentioned this pull request Jan 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@noisysocks noisysocks noisysocks approved these changes

@aduth aduth aduth left review comments

Assignees
No one assigned
Labels
[Feature] Block Multi Selection The ability to select and manipulate multiple blocks [Feature] Writing Flow Block selection, navigation, splitting, merging, deletion... [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
4 participants
@jorgefilipecosta @noisysocks @aduth @designsimply