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]

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.

[trevinhofmann]

@raineorshine -- It seems that iOS Safari does not provide any events to detect when the keyboard is open/closed or opening/closing. That explains why we have the workaround of using selection.isActive() to detect when the keyboard is open, but it limits our ability to know when the keyboard is finished closing.

My proposed solution (e06af0a) refactors the keyboard detection out of usePositionFixed into a dedicated safariKeyboardStore and uses a fixed timeout to determine when the keyboard has completed closing.

Please let me know if you have any better ideas, but I haven't been able to find any way to definitively determine when the keyboard finishes closing.

Demo:

Screen.Recording.2025-02-15.at.6.12.06.PM.mov

[trevinhofmann]

One potential enhancement would be to use a setInterval rather than setTimeout to gradually update the state of the "keyboard height" for iOS Safari to mimic it reducing in height as it closes. This way, the modal could slide down with the keyboard rather than temporarily disappearing. Again, though, there appears to be no supported way of determining the exact timing/height of the iOS Safari keyboard. The calculated/estimated keyboard height might be slightly different from the actual keyboard height as it closes.

If you'd like me to try this approach, please just let me know.

[raineorshine]

Thanks @trevinhofmann!

@raineorshine -- It seems that iOS Safari does not provide any events to detect when the keyboard is open/closed or opening/closing. That explains why we have the workaround of using selection.isActive() to detect when the keyboard is open, but it limits our ability to know when the keyboard is finished closing.

Yes, it's unfortunate there isn't a native event for this. We have state.editing in the Redux store which basically tracks the virtual keyboard on mobile. However, it's a bit more indirect in that it's tied in with the editor. It is also causative, actually determining whether the virtual keyboard is opened or not depending on certain commands (e.g. newThought, clearThought always turn editing mode on). It may be safer to maintain a separate implementation for the position fixed functionality, although I don't love the redundancy. Thoughts?

My proposed solution (e06af0a) refactors the keyboard detection out of usePositionFixed into a dedicated safariKeyboardStore and uses a fixed timeout to determine when the keyboard has completed closing.

That seems fine.

One potential enhancement would be to use a setInterval rather than setTimeout to gradually update the state of the "keyboard height" for iOS Safari to mimic it reducing in height as it closes. This way, the modal could slide down with the keyboard rather than temporarily disappearing. Again, though, there appears to be no supported way of determining the exact timing/height of the iOS Safari keyboard. The calculated/estimated keyboard height might be slightly different from the actual keyboard height as it closes.

It's an interesting idea, but I don't trust being able to accurately track the virtual keyboard position.

We could "anticipate" the new height and render the alert in the correct place ahead of time, but I think that should be explored in a separate PR, if at all.

---

I have a few comments/questions on the code before I test.

[raineorshine]

I prefer to initialize document and events in a dedicated loading phase rather than in the module scope. It creates more predictable timing, and allows event listeners to be removed in a cleanup phase if needed.

Therefore, I recommend exporting updateKeyboardState and calling it in the existing global onSelectionChange:

em/src/util/initEvents.ts

Lines 164 to 171 in 71545c5

	/** Selection change event listener; save selection offset to storage, update command state store. */
	const onSelectionChange = () => {
	// save selection offset to storage, throttled
	saveSelectionOffset()
	// update command state store
	updateCommandState()
	}

[trevinhofmann]

Yes, that is understandable. Refactored here: fda65a3

[raineorshine]

Can we use innerHeight - virtualKeyboardHeight instead of currentKeyboardHeight?

[trevinhofmann]

@yangchristina might be better able to answer this, but the virtualKeyboardHeight seems to be a cached estimate in this case, whereas currentKeyboardHeight is correctly calculated. In testing, the virtualKeyboardHeight was ~290px and currentKeyboardHeight was ~268px. Using the currentKeyboardHeight seems to give a better positioning in this case.

[raineorshine]

Sorry, my statement was ambiguous. I meant the state variable, state.virtualKeyboardHeight, which is:

        isViewportValid
          ? virtualKeyboardHeight
          : isPortrait
            ? virtualKeyboardHeightPortrait
            : virtualKeyboardHeightLandscape`

I didn't realize virtualKeyboardHeight was the name of both a local variable and a state variable.

@yangchristina might be better able to answer this, but the virtualKeyboardHeight seems to be a cached estimate in this case, whereas currentKeyboardHeight is correctly calculated.

The main problem is that currentKeyboardHeight uses window.visualViewport.height as-is, which has a bug in iOS Safari. When the phone is rotated with the keyboard open, rotated back, and then the keyboard closed, the value of window.visualViewport.height is wrong. We have to override it to the known portrait or landscape height based on the device dimensions. That is the intention behind state.virtualKeyboardHeight.

The estimates are only used until a valid viewport is detected, then they permanently change to the measured height.

In testing, the virtualKeyboardHeight was ~290px and currentKeyboardHeight was ~268px. Using the currentKeyboardHeight seems to give a better positioning in this case.

I'm not sure if you're talking about the local variable or state variable (again, my bad on the ambiguity), but I'm assuming these measurements were not taken under the condition of the rotation bug, therefore window.innerHeight - window.visualViewport.height will be correct.

Regardless, state.virtualKeyboardHeight should be preferred over window.innerHeight - window.visualViewport.height due to the rotation bug. If it's giving you bad numbers, and the existing code has a bug, I can only speculate as to the cause:

• It is properly switching from the estimated height to the measured height?
• Are the hardcoded ratios for virtualKeyboardHeightPortrait incorrect for some devices?
• Is isViewportValid correct?

Sorry for the added trouble. I just want to make sure the rotation bug doesn't sneak in to currentKeyboardHeight.

[trevinhofmann]

There may be an issue with virtualKeyboardHeight.

Here are screenshots showing an alert with the current values of currentKeyboardHeight and virtualKeyboardHeight from the store/state, using each of those values as either:

      ? `${scrollTop + innerHeight - currentKeyboardHeight - (height ?? 0) - offset}px`

      ? `${scrollTop + innerHeight - virtualKeyboardHeight - (height ?? 0) - offset}px`

Using currentKeyboardHeight	Using virtualKeyboardHeight

Also, this seems to be the only place that we are using currentKeyboardHeight. If we do replace it here in the calculation, I believe we could remove currentKeyboardHeight from the store entirely.

---

Regarding the portrait/landscape rotation, the current implementation doesn't seem to cause issues when using currentKeyboardHeight (after changing to state.editing):

ios.rotation.mp4

[raineorshine]

Wouldn't currentKeyboardHeight always be 0 in this case, since the keyboard is closed?

[trevinhofmann]

Yep! Refactored that one out here: 2887108

[trevinhofmann]

Yes, it's unfortunate there isn't a native event for this. We have state.editing in the Redux store which basically tracks the virtual keyboard on mobile. However, it's a bit more indirect in that it's tied in with the editor. It is also causative, actually determining whether the virtual keyboard is opened or not depending on certain commands (e.g. newThought, clearThought always turn editing mode on). It may be safer to maintain a separate implementation for the position fixed functionality, although I don't love the redundancy. Thoughts?

When changing the landscape/portrait rotation, I noticed that selection.isActive() does not provide the correct value when closing the keyboard after the rotation.

This is resolved by switching to state.editing, and I've made that change here: c16fd5c