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

Remove the blink_input_opacity_to_prevent_scrolling_when_focus hack #18266

Merged
merged 2 commits into from
May 2, 2023
Merged

Remove the blink_input_opacity_to_prevent_scrolling_when_focus hack #18266

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
60f8e13
Remove the blink_input_opacity_to_prevent_scrolling_when_focus hack
cubuspl42 Apr 30, 2023
12ee87f
index.html: Remove the comment about interactive-widget
cubuspl42 May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions web/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,9 @@
outline: 0;
box-shadow: inset 0px 0px 0px 1px #5AB0FF;
}
@keyframes blink_input_opacity_to_prevent_scrolling_when_focus {
0% { opacity: 0; }
100% { opacity: 1; }
}
input:focus-visible, input:focus[data-focusvisible-polyfill],
select:focus-visible, select:focus[data-focusvisible-polyfill] {
box-shadow: none;
animation: blink_input_opacity_to_prevent_scrolling_when_focus 0.01s;
}
::-ms-reveal {
display: none;
Expand All @@ -81,6 +76,13 @@
caret-color: #ffffff;
}
</style>
<!--
Note: interactive-widget=resizes-content is ignored on iOS Safari. Working around that has proven to be
problematic and causing bugs. At the moment of writing, we don't really need to work around this. If we attempt
to do that again, we should do that carefully and minding the potential side effects.

Some context can be found in https://github.com/Expensify/App/issues/16082
-->
<meta name="viewport" content="width=device-width, user-scalable=no, initial-scale=1, interactive-widget=resizes-content">
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we even need interactive-widget=resizes-content here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's respected on iOS Chrome, if I recall correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, but do we even need it? This was also added to prevent auto scroll, and auto scroll is not our enemy 😅 so maybe we should remove that code too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The interactive-widget property doesn't affect auto-scroll. This affects viewport behavior when the virtual keyboard is shown.

https://developer.chrome.com/blog/viewport-resize-behavior/

resizes-content is what we want. It's supported mainly (or only?) by Chrome, but it's present in CSS Viewport Module Level 1 Editor’s Draft, so one day maybe even iOS Safari will support it.

So it's unrelated and there's no need to change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please remove the comment then, I don't think we need explain anything here, it may be just confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We're removing a hack that was a workaround for the fact that Safari doesn't support that property. So that's why I left a comment. But I'll remove it if you think it's confusing

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I dunno, I think the comment is fine. This is a file we don't touch much, so I don't mind leaving it there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@s77rt Done

<link rel="shortcut icon" id="favicon" href="/favicon.png">
<% if (htmlWebpackPlugin.options.usePolyfillIO) { %>
Expand Down