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

Mobile: Fix toolbar position #6307

Closed
wants to merge 1 commit into from
Closed

Conversation

youknowriad
Copy link
Contributor

closes #6236

The mobile toolbar should always be shown as fixed.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended Mobile Web Viewport sizes for mobile and tablet devices labels Apr 20, 2018
@youknowriad youknowriad self-assigned this Apr 20, 2018
@youknowriad youknowriad requested a review from jasmussen April 20, 2018 08:36
@karmatosed
Copy link
Member

Previously we didn't have this option as it was broken, if this now isn't as broken as in #4079 totally ok with it being back. That was a force to get around the visual issues in past.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

From the code point of view changes here look good. I tested and it has the correct functionality, it forces the toolbar to be on the top on mobile as expected.
Given that now it is possible to show toolbar at the top on mobile I wonder if now we should not just let mobile users choose their preference like desktop users can do.

@jasmussen
Copy link
Contributor

Thanks for working on this, and thanks for the context, Tammie. It looks like the problem that the switch was intended to solve hasn't been fixed:

now

You still have to scroll to the top. But at least now you can select text and use the bold/italic, without covering them with the cut/copy/paste iOS selection thing.

I'm gong to take a look at fixing the toolbar so it's always visible — it should be sticky. Going to try and see if I can push a fix to this branch.

@jasmussen
Copy link
Contributor

So as it turns out, we might need a "clever trick" for this. The problem, which I hoped moving the toolbar to the top would solve, was that the popup cut/copy/paste dialog in iOS covered the block toolbar. The problem is as soon as the soft keyboard shows up on iOS, well essentially position: sticky and position: absolute stop working.

Here's what Notion does:

notion

It's basically a popup toolbar that shows up just on setting the cursor. Not ideal. I'm attempting some of the hacks suggested in https://gist.github.com/avesus/957889b4941239490c6c441adbe32398.

As a reminder, this is what master looks like right now:

master

Note how words on line 1 are blocked by the ios popup toolbar.

@jasmussen
Copy link
Contributor

So, after investigating this for a bit, I believe there are only a few options on our table. This is keeping in mind that it feels as if mobile safari is simply not designed to allow writing experiences — Google Docs, Medium both block writing in mobile browsers and require you to use the app.

The first option is to keep the toolbar fixed to the top, and... do nothing. Simply accept that you have to scroll aaaaall the way up there if you need to make text bold, or create links. Not ideal. This is what WordPress.com does now, this is what the classic editor does now:

classic editor

The other option is to enter serious hack terrritory. It involves us blocking all touchstart and touchend calls on iOS, so you can't scroll the html element, then calculating the keyboard height, and setting the viewport hight to be the remaining available space. Supposedly possible to get right, but REALLY REALLY difficult, and probably subject to change at the whim of Safari updates.

The third option is to look at what Notion.so has done, and either do the same — create a "popup toolbar" that sits below the cursor.

Or, we can rearrange the block toolbar to be below the block, so you can't cover it with the iOS cut/copy/paste bubble. Something like this:

block-toolbar-below

It's not ideal, but it feels like the solution that will work the best for us.

jasmussen pushed a commit that referenced this pull request Apr 23, 2018
This is very much an experiment, intended to help solve problems discovered in #6307 (comment).

Basically, on iOS you won't have access to the block toolbar until you scroll way up to the top. And if we dock the toolbar to the top of the block, it will be covered by cut/copy/paste. This PR moves the toolbar below the block instead, mitigating that.
@jasmussen
Copy link
Contributor

Took a stab at a different solution in #6347.

@gziolo
Copy link
Member

gziolo commented May 10, 2018

Should we close this one as it seems like we are going with #6347?

@youknowriad youknowriad deleted the fix/mobile-toolbar-position branch May 10, 2018 08:05
jasmussen added a commit that referenced this pull request May 16, 2018
This is very much an experiment, intended to help solve problems discovered in #6307 (comment).

Basically, on iOS you won't have access to the block toolbar until you scroll way up to the top. And if we dock the toolbar to the top of the block, it will be covered by cut/copy/paste. This PR moves the toolbar below the block instead, mitigating that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mobile Web Viewport sizes for mobile and tablet devices [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression with mobile formatting toolbar
5 participants