V1.5 Alert

[yangchristina]

No description provided.

[raineorshine]

Thanks for your submission!

The main Alert component is represented by https://github.com/cybersemics/em/blob/main/src/components/Popup.tsx. https://github.com/cybersemics/em/blob/main/src/components/Alert.tsx is a HOC that mainly just handles fade in/out. By circumventing Popup.tsx we're losing some key functionality:

• Cross-browser support for position: fixed
• Swipe-to-dismiss functionality
• Multicursor support
• Import Files special handling.

The goal is to re-style the alert without losing any of the existing functionality. I would suggest duplicating https://github.com/cybersemics/em/blob/main/src/components/Popup.tsx and using that as a starting point. The only other component that uses it is

em/src/components/CommandPalette.tsx

Line 469 in 42b1ef5

<Popup

, and that will be re-styled as well so we could make them separate components to decouple them.

Let me know if this makes sense!

[raineorshine]

Could we add an ✗ that appears in the upper right corner on hover to dismiss the alert? I realize the mockup did not account for the close functionality on desktop.

I'm okay if this is + time.

[yangchristina]

Yes I can add it.

[yangchristina]

Thanks for your submission!

The main Alert component is represented by https://github.com/cybersemics/em/blob/main/src/components/Popup.tsx. https://github.com/cybersemics/em/blob/main/src/components/Alert.tsx is a HOC that mainly just handles fade in/out. By circumventing Popup.tsx we're losing some key functionality:

• Cross-browser support for position: fixed
• Swipe-to-dismiss functionality
• Multicursor support
• Import Files special handling.

The goal is to re-style the alert without losing any of the existing functionality. I would suggest duplicating https://github.com/cybersemics/em/blob/main/src/components/Popup.tsx and using that as a starting point. The only other component that uses it is

em/src/components/CommandPalette.tsx

Line 469 in 42b1ef5

<Popup

, and that will be re-styled as well so we could make them separate components to decouple them.
Let me know if this makes sense!

Should font size and padding still follow these?

  const fontSize = useSelector(state => state.fontSize)
  const padding = useSelector(state => state.fontSize / 2 + 2)

[yangchristina]

I'll update the puppeteer snapshot once the styling has been confirmed.

[raineorshine]

Let's go with scaling the fontSize and em units for margin and padding. If we use scale on the Alert component but not other components, I think it's going to cause small aspects like borders to be inconsistent.

[yangchristina]

Let's go with scaling the fontSize and em units for margin and padding. If we use scale on the Alert component but not other components, I think it's going to cause small aspects like borders to be inconsistent.

Sounds good!

[trevinhofmann]

Thanks for the PR, @yangchristina!

Quick note: Please include a PR description explaining the goals of the change. I have the design document for these UI/UX changes, but it would still be nice to know a bit more about which particular parts the PR is changing.

---

1. The "x" button does not close the alert

The "x" button appears correctly on hover (although it's a bit small, IMO), but clicking it does not dismiss the alert popup.

Screen.Recording.2025-01-06.at.5.45.20.PM.mov

---

2. Confusing cursor pointer on icons

This might be intentional and a non-issue, but it was confusing to me that the cursor changes to a pointer when hovering over the icon. Normally, the pointer is reserve for clickable items that cause an action, but this icon is not clickable.

Screen.Recording.2025-01-06.at.5.46.31.PM.mov

---

On mobile, I have a few questions that I would just like to clarify (cc @raineorshine).

A) Should the alert be centered, or is it intended to be on the right side of the screen? I don't see a mobile design for the alert in Figma.
B) Should the alert still fade out / dismiss while I'm actively dragging it?
C) Should the alert be dismissed by dragging it up/down, or left/right? I'm fine with either, but my initial "training" as a user made me assume that it would be dismissed by dragging it left/right.

alert.mp4

[yangchristina]

@trevinhofmann Here's the figma, it is supposed to centred.

There aren't mocks for the 'x', so I didn't know how large to make it. I can make it larger. @raineorshine what size would be good?

[yangchristina]

@raineorshine for the 'x' button, should it only show when alert?.showCloseLink is true?

[raineorshine]

A) Should the alert be centered, or is it intended to be on the right side of the screen? I don't see a mobile design for the alert in Figma.

Centered

B) Should the alert still fade out / dismiss while I'm actively dragging it?

I don't think it needs to fade out while actively dragging it. I'm basing this on the current behavior in main.

C) Should the alert be dismissed by dragging it up/down, or left/right? I'm fine with either, but my initial "training" as a user made me assume that it would be dismissed by dragging it left/right.

Since it shows up at the bottom of the screen, my tendency is to dismiss it by dragging it down. Let's stick with down for now, but I definitely understand the left/right tendency as well. I'd love to do some user testing on this to see what is most intuitive for most people.

There aren't mocks for the 'x', so I didn't know how large to make it. I can make it larger. @raineorshine what size would be good?

We can follow the mockup for Tip: https://www.figma.com/design/g0osrpkb5dhY0qPxW9mWjB/Em-UX?node-id=227-886&t=qhUgnmnLKgPDUViF-4

A couple points:

• There is additional padding to make room for the ✗ button. This should be applied to both sides for balance.
• The tappable area should extend slightly beyond the visible ✗, just like in main.

[yangchristina]

Does this look around right? The icon in the mocks is thinner than the text, but CloseButton uses text instead of an svg icon.

Spacing wise, using text is less exact for vertical padding.

@raineorshine what do you think of switching the close button to the svg icon in the figma mocks instead of the text character?

[raineorshine]

That does look close to the mockup, although I didn't anticipate how it would look with a single-line alert. I don't think the vertical centering looks great for a close button, which is expected to be in the upper right.

I was thinking more like this:

Or we could move it to the corner in a circle, which I think looks a bit better:

I could ask Ahmad to do a mockup of the latter so you have more exact proportions to work with.

I'm fine with using SVG for it if that helps.

[raineorshine]

I don't think there's a simple way to move the alert up when the keyboard opens. However I can make it so that if the keyboard is open, you can see the alert. However if the alert was there before the keyboard opened, it will get covered once the keyboard opens. Is this okay @raineorshine?

The alert is also used for ongoing states such as drag-and-drop, multicursor, import progress, etc. I'm afraid that if the keyboard covers the alert, the user won't have this feedback.

Another problem is the inverse: If the user triggers an alert while the keyboard is up, and then closes the keyboard, the alert is left floating in the middle of the screen.

In both cases we do need a true position: fixed at the bottom of the viewport, above the keyboard.

Instead of using window.visualViewport, which is non-reactive (and buggy), my suggestion is to use our viewportStore.

const { innerHeight, virtualKeyboardHeight } = viewportStore.useState()

@raineorshine I've also updated the close button svg. Does this look right?

I haven't tested it at multiple font sizes, but looks great so far! A few suggestions:

• I think the padding of the close button in the mockup looks a little small. I bumped the width from 20 to 24 in the mockup. Could you increase the padding accordingly?
• Let's make it desktop only. Mobile fires a hover event on click ("click intent") which we don't want in this case.
• Expand the click area about 10px on all sides to make it easier to click. I typically do this with a combination of padding and negative margin.
• Ensure that the expanded click area is available at all times, not just on hover. Otherwise hovering a few pixels above the upper-right corner where the ✗ should appear won't do anything initially.

[trevinhofmann]

Hi, @yangchristina! I will delay reviewing for now until you have a chance to address the latest feedback from @raineorshine. Please let me know when this is ready again for review, and I'll be happy to have another look. Thank you!

[yangchristina]

@raineorshine viewportStore doesn't update when keyboard is up, so it doesn't fix this. I can't find a solution to this (tried touchmove too but doesn't work)
(using position: fixed bottom will not show when keyboard comes up, so I need to to add the keyboard height when the keyboard comes up. To do this I need some sort of listener for when the keyboard comes up)

[raineorshine]

Thanks for the update. I didn't realize viewportStore was not updated when the virtual keyboard activated. We should be able to update it in response to the selectionchange event, when selection.isActive() changes. Then innerHeight will be accurate.

If that doesn't work, or if you need additional guidance, @trevinhofmann can work with you on a solution. Thanks!

[yangchristina]

@raineorshine actually it kinda works sometimes? There was one point where the alert was actually working, but now it's back to not really working. (didn't make any code changes, just seems really inconsistent)

Screen.Recording.2025-01-30.at.12.35.13.AM.mov

[yangchristina]

Here's another example of it being really inconsistent. I'm not sure if it's just a problem with the simulator. From the logs, viewportStore does actually update correctly, but changes don't always show, or show inconsistently

Screen.Recording.2025-01-30.at.12.39.15.AM.mov

[raineorshine]

That is strange. Some ideas:

1. Confirm that XCode Simulator does not behave differently from physical phone.
2. Confirm viewportStore.update is called whenever the keyboard changes.
3. Confirm Alert is re-rendered correctly when the window size changes.

There is also a question of timing. We don't want the Alert position to be delayed, and it's probably not possible for it to smoothly animate down with the keyboard. Our best bet is probably to have the Alert snap to the bottom as soon as the keyboard is closed. The keyboard should cover the alert while animating out of view. Then the Alert will be in its place at the bottom of the screen.

However, it's not really possible to design the correct behavior when you are getting inconsistent behavior, so I would focus on understanding the cause of the inconsistency first. There is usually another variable we are not aware of that is affecting the behavior.

Hope that helps!

[yangchristina]

@raineorshine I don't really have a way of running this on an actual phone. (I don't use an iphone and even if I did I don't know how I'd connect that phone to em's localhost)

[raineorshine]

No problem, one of us can verify that. It's unlikely that it's the problem, but it doesn't hurt to verify base assumptions when inconsistent behavior is observed.

[yangchristina]

@raineorshine that would be great, thanks!

[yangchristina]

2. Confirm viewportStore.update is called whenever the keyboard changes.

Can confirm this. It does update when keyboard changes.

3. Confirm Alert is re-rendered correctly when the window size changes.

How can I resize on ios devices?

[trevinhofmann]

Running the latest on a physical iPhone, I see the same functionality. It looks close to working, but there is a lagging update to the positioning when the keyboard is opened/closed. I will be able to look into this more later today.

alert-lag.mp4

[raineorshine]

How can I resize on ios devices?

You would rotate from portrait to landscape. XCode Simulator has an option to do this also.

[raineorshine]

Running the latest on a physical iPhone, I see the same functionality. It looks close to working, but there is a lagging update to the positioning when the keyboard is opened/closed. I will be able to look into this more later today.

When the keyboard is closed, you may need to "anticipate" the upcoming screen height in order to render the alert at the bottom instead of animating it down. That’s the tricky part.

[trevinhofmann]

For the sake of debugging, I:

• added a permanent alert to show the keyboard height from the viewport store
• added a secondary alert at the top (to always have debugging visible)
• added the usePositionFixed styles for the regular alert, to both alerts

a-popup-1.mp4

I made the style text quite small to ensure it fits, but it should still be visible in the video when expanded.

This debugging revealed that:

1. The keyboard height is always accurate (though a bit delayed when closing the keyboard, which may be unavoidable)
2. The usePositionFixed for the alert switches from position: fixed to position: absolute when the keyboard is opened, and this positioning seems to prevent it from rendering correctly above the keyboard

I'm not well informed on the history/reasoning for the position: absolute functionality. The git blame history is a bit muddled, but the comments indicate this is a fix for mobile Safari. Considering the position: fixed seems to consistently work on iOS Safari (likely thanks to the changes in this PR), perhaps we can remove it? I would just need to do some more testing to make sure there are no unintended side effects for other components.

As a (very tentative) fix, this commit removes the position: absolute functionality from the usePositionFixed hook: 6b4edd7

With that change added (and making the alert permanent for demo purposes), the alert seems to be correctly positioned. There is still a delay when closing the keyboard, but that may be unavoidable. It seems that Safari does not trigger the resize event until the keyboard fully closes.

a-popup-2.mp4

[raineorshine]

Thank you for the additional testing @trevinhofmann!

I'm not well informed on the history/reasoning for the position: absolute functionality. The git blame history is a bit muddled, but the comments indicate this is a fix for mobile Safari. Considering the position: fixed seems to consistently work on iOS Safari (likely thanks to the changes in this PR), perhaps we can remove it? I would just need to do some more testing to make sure there are no unintended side effects for other components.

As far as I know, position: fixed doesn't work at all on mobile Safari when the virtual keyboard is up. The element will stay fixed to the bottom of the document instead of the bottom of the viewport.

Demo: https://s5ntpd.csb.app/

Let me know if you are seeing something different. I've worked on the problem at various times over the years and have not been able to find a proper solution. Most recently I generalized and factored out the the position: fixed functionality into the usePositionFixed hook so it can be used in the Toolbar, Alert, and other components as needed.

There is still a delay when closing the keyboard, but that may be unavoidable. It seems that Safari does not trigger the resize event until the keyboard fully closes.

That's too bad. I don't think the delay is really acceptable from a UX perspective. The alert floating in the middle of the screen looks pretty bad.

I was thinking about the challenge of "anticipating" the next height, and perhaps the better approach is to simply hide the alert while the virtual keyboard is closing. We could hide it on blur (or global selectionchange) and then show it again when the resize fires.

Let me know what you think. Thanks!

[trevinhofmann]

As far as I know, position: fixed doesn't work at all on mobile Safari when the virtual keyboard is up. The element will stay fixed to the bottom of the document instead of the bottom of the viewport.

Ah! That makes sense. I see the issue when scrolling. In that case, we'll need to fix the calculations for position: absolute in usePositionFixed (rather than removing them), as that seems to be the problem.

I was thinking about the challenge of "anticipating" the next height, and perhaps the better approach is to simply hide the alert while the virtual keyboard is closing. We could hide it on blur (or global selectionchange) and then show it again when the resize fires.

That makes sense to me!

---

@yangchristina -- Would you like to work on the above two changes? Otherwise, I will be available to attempt them later today.

[yangchristina]

As far as I know, position: fixed doesn't work at all on mobile Safari when the virtual keyboard is up. The element will stay fixed to the bottom of the document instead of the bottom of the viewport.

Ah! That makes sense. I see the issue when scrolling. In that case, we'll need to fix the calculations for position: absolute in usePositionFixed (rather than removing them), as that seems to be the problem.

I was thinking about the challenge of "anticipating" the next height, and perhaps the better approach is to simply hide the alert while the virtual keyboard is closing. We could hide it on blur (or global selectionchange) and then show it again when the resize fires.

That makes sense to me!

@yangchristina -- Would you like to work on the above two changes? Otherwise, I will be available to attempt them later today.

@trevinhofmann Sorry for responding late. Position absolute is already being handled though, so that shouldn't be the issue.

  if (position === 'absolute') {
    top = fromBottom ? `${scrollTop + innerHeight - (height ?? 0) - offset}px` : `${scrollTop + offset}px`
  } else if (fromBottom) {
    bottom = `calc(${token('spacing.safeAreaBottom')} + ${offset + currentKeyboardHeight}px)`
  } else {
    top = `calc(${token('spacing.safeAreaTop')} + ${offset}px)`
  }

[trevinhofmann]

@trevinhofmann Sorry for responding late. Position absolute is already being handled though, so that shouldn't be the issue.

No problem! I'm suspicious that some part of the position: absolute calculation is incorrect since that seems to be when the alert is not visible (from the first video in #2746 (comment)).

I will let you take the first attempt at resolving this, but please let me know if you are stuck and would like some help.

Thank you!

[yangchristina]

@trevinhofmann Sorry for responding late. Position absolute is already being handled though, so that shouldn't be the issue.

No problem! I'm suspicious that some part of the position: absolute calculation is incorrect since that seems to be when the alert is not visible (from the first video in #2746 (comment)).

I will let you take the first attempt at resolving this, but please let me know if you are stuck and would like some help.

Thank you!

Sounds good, thanks! Do you happen to have the code for the debugging somewhere? If it's not too inconvenient, would love to see how you did this.

[yangchristina]

Fixed! Yes, my calculations for position: absolute were wrong. innerHeight inviewportStore said "Height of the viewport, not including the virtual keyboard.", which I assumed meant the full height minus the keyboard. I've updated the jsdoc comment to "Height of the viewport, including the virtual keyboard." if that's okay. I can either do that or subtract keyboard height from innerHeight and keep the jsdoc comment as is.

I was thinking about the challenge of "anticipating" the next height, and perhaps the better approach is to simply hide the alert while the virtual keyboard is closing. We could hide it on blur (or global selectionchange) and then show it again when the resize fires.

UX wise it seems fine to me to have a delay when you are opening the keyboard, but not when you are closing it. The delay when opening is essentially the same as hiding the alert a bit before showing it again. But for closing, having it float in the middle of the screen is more bothersome and looks like a bug.

The original scope of this alert was just supposed to include styling, so changing the way it shows on keyboard close is out of scope.

[yangchristina]

I'm not sure why, but ColorPicker tests aren't failing for me locally.

[raineorshine]

Fixed! Yes, my calculations for position: absolute were wrong. innerHeight inviewportStore said "Height of the viewport, not including the virtual keyboard.", which I assumed meant the full height minus the keyboard. I've updated the jsdoc comment to "Height of the viewport, including the virtual keyboard." if that's okay. I can either do that or subtract keyboard height from innerHeight and keep the jsdoc comment as is.

You are correct! Sorry for the confusion. I clearly documented that wrong at some point. innerHeight does include the virtual keyboard. virtualKeyboardHeight can be subtracted from it to get the visible viewport height.

This might be the source of other bugs if there is any more code that is assuming the wrong behavior of innerHeight, but I will have to investigate that separately.

UX wise it seems fine to me to have a delay when you are opening the keyboard, but not when you are closing it. The delay when opening is essentially the same as hiding the alert a bit before showing it again. But for closing, having it float in the middle of the screen is more bothersome and looks like a bug.

Yes, I agree.

The original scope of this alert was just supposed to include styling, so changing the way it shows on keyboard close is out of scope.

Thanks, agreed.

@trevinhofmann Could you possibly do this part since you are familiar with the problem? I would prefer do it all in one PR so that we can avoid the layout problem getting into main.

I'm not sure why, but ColorPicker tests aren't failing for me locally.

Nothing changed with it, but I've been noticing a lot more GitHub Actions failures due to this snapshot test. I have temporarily disabled it in d936436 until we can get to the bottom of it. Thanks.