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

New popup position means popup position varies based on size of popup #756

Closed
nlgiles opened this issue Sep 11, 2021 · 11 comments · Fixed by #759
Closed

New popup position means popup position varies based on size of popup #756

nlgiles opened this issue Sep 11, 2021 · 11 comments · Fixed by #759

Comments

@nlgiles
Copy link

nlgiles commented Sep 11, 2021

After an update to the popup positioning, the popup now gets shifted up or down to try to fit the whole thing on screen. I get the reasoning behind this, but in some of my use cases it's quite troublesome. When using 10ten to highlight multiple words in a sentence, the position of top of the pop-up changes wildly from word-to-word, meaning the on-screen location of the top-most definition changes. For example, if you are highlighting several words all on the same line of text in the middle of the screen, as you move from one word to the next, the position of the top-most definition on the screen can vary from below the highlighted word if the whole box fits below the word, to the top of your screen if the box is big enough to fill the screen. This gives me a very cumbersome feel when I highlight a word in the middle of my screen and the top-most definition appears in a wildly different place on screen each time. Its extremely noticeable when I pass over particles while moving my mouse from one word to another because almost all particle popups are huge and fill my screen top to bottom. Before this change I would just always read text in the top half of my screen and the popup would consistently appear in an expected place beneath the highlighted words.

I think the best solution would be a scrollable popup with the same positioning code as before, so that when the popup is too large you can hover it and scroll down, but I think that might be troublesome. I would appreciate if if there were an option to force the popup to always appear below/right of the word being highlighted even if that causes the popup to get cut off, or at least just an option to use the old positioning code.

@birtles
Copy link
Member

birtles commented Sep 11, 2021

After an update to the popup positioning, the popup now gets shifted up or down to try to fit the whole thing on screen. I get the reasoning behind this, but in some of my use cases it's quite troublesome. When using 10ten to highlight multiple words in a sentence, the position of top of the pop-up changes wildly from word-to-word, meaning the on-screen location of the top-most definition changes.

Yes, I've noticed this too and I tend to agree.

The reasoning for the change is to try to do a better job with touch screen devices where you can't just scroll by spinning the mouse wheel.

For example, if you are highlighting several words all on the same line of text in the middle of the screen, as you move from one word to the next, the position of the top-most definition on the screen can vary from below the highlighted word if the whole box fits below the word, to the top of your screen if the box is big enough to fill the screen. This gives me a very cumbersome feel when I highlight a word in the middle of my screen and the top-most definition appears in a wildly different place on screen each time. Its extremely noticeable when I pass over particles while moving my mouse from one word to another because almost all particle popups are huge and fill my screen top to bottom. Before this change I would just always read text in the top half of my screen and the popup would consistently appear in an expected place beneath the highlighted words.

Honestly I don't recall how the old popup positioning worked exactly because we certainly put the popup above the text sometimes but I'll have to look up exactly what those circumstances were. I tend to think that original heuristic worked pretty well.

At very least, I think we should probably avoid putting the popup on the side as I mentioned in #752 (comment)

I think the best solution would be a scrollable popup with the same positioning code as before, so that when the popup is too large you can hover it and scroll down, but I think that might be troublesome.

We definitely want to add the scrollable part, at least for touchscreen devices. Making the scrolling work for mouse users will be a bit harder but is something we want to do eventually.

I would appreciate if if there were an option to force the popup to always appear below/right of the word being highlighted even if that causes the popup to get cut off, or at least just an option to use the old positioning code.

Yes, I'll look into making the positioning a bit more compatible with what we used to have.

Note that there is the option to turn on the j / k keys so you can force the popup to always be in, e.g. the bottom right, but that probably doesn't do exactly what you want.

/cc @shirakaba

@nlgiles
Copy link
Author

nlgiles commented Sep 11, 2021

Honestly I don't recall how the old popup positioning worked exactly because we certainly put the popup above the text sometimes but I'll have to look up exactly what those circumstances were. I tend to think that original heuristic worked pretty well.

I don't know how the code worked directly, but my guess based on my experience was that it preferred to put the pop-up below unless both were true:

  1. It didn't entirely fit below.
  2. There was more screen space above than below.

My experience was that as long as the text I was highlighting was in the top half of my screen (so there was always more screen space below) then the popup would always appear below.

@birtles
Copy link
Member

birtles commented Sep 11, 2021

I don't know how the code worked directly, but my guess based on my experience was that it preferred to put the pop-up below unless both were true:

  1. It didn't entirely fit below.
  2. There was more screen space above than below.

Yeah, you're right. It would choose the side based on which one has more room:

// Check if we are too close to the window edge (bottom / right)...
let block = isVerticalText ? x : y;
const blockPopupSize = isVerticalText ? popupSize.width : popupSize.height;
const blockWindowSize = isVerticalText ? windowWidth : windowHeight;
let constrainBlockSize: number | null = null;
if (block + blockAdjust + blockPopupSize > blockWindowSize) {
// ...we are. See if the other side has more room.
const spaceOnThisSide = blockWindowSize - block;
const spaceOnOtherSide = block;
if (spaceOnOtherSide > spaceOnThisSide) {
blockAdjust = Math.max(-blockPopupSize - 25, -block);
if (spaceOnOtherSide - 25 < blockPopupSize) {
constrainBlockSize = spaceOnOtherSide - 25;
}
}
}

Whereas now we do it based on the area of the popup (at least in the case where we find there's no unconstrained position in the block direction).

I'm not sure what will be best for smartphones and other touchscreen devices but at least for mouse-based desktop devices I think we probably want to alter the behavior to:

  • Always use one of the the block direction positions.
  • Choose the side based on the available space (preferring below if it fits).

@birtles
Copy link
Member

birtles commented Sep 11, 2021

Just fiddling with my iPhone it doesn't ever seem to use the inline positions (left/right) so I wonder if we really need those after all. If we dropped them, we could possibly drop the complicated code we have to make sure we don't overlap the text on left/right.

@birtles
Copy link
Member

birtles commented Sep 11, 2021

I'm going to give @shirakaba a chance to chime in before jumping in to this but I agree this is a regression we should fix as soon as possible.

@shirakaba
Copy link
Collaborator

  • Always use one of the the block direction positions.

This will tend to work fine on tall, narrow, windows, but on wide, squat screens (e.g. landscape on a phone, although I’ll admit not many people browse the internet like that often), we do need left/right sides as an option.

  • Choose the side based on the available space (preferring below if it fits).

Not quite sure how this will differ from our area-based selection, particularly as we reserve extra space above/below to dodge the thumb.

I can probably think about it more after I’m back home.

@shirakaba
Copy link
Collaborator

shirakaba commented Sep 11, 2021

@nlgiles Thanks for the report – maximising readability is a top concern.

Note that in the below comments, I'm thinking principally about horizontal text and laying out the popup above vs. below, and am not really thinking much about laying it left vs. right and considerations for vertical text.

The broad aim

I think @nlgiles's sentiment is broadly that we should aim to keep a constant reading line each time the popup appears? i.e. each time the popup appears, we don't want to have to search around with our eyes for where the first definition starts.

The original layout behaviour

I'm actually surprised that this was ever achieved with the old layout code, but as @nlgiles confirms:

Before this change I would just always read text in the top half of my screen and the popup would consistently appear in an expected place beneath the highlighted words.

Indeed, when the popup spawns below, it keeps the same reading line. So given equal or unlimited space on each side of the lookup position, spawning below should certainly be preferred.

Considerations for mobile devices

Unfortunately, on mobile devices, window space is very limited, and often we have no choice but to place the popup above the lookup position. Moreover, whenever the popup is placed below the lookup position, in practice, we need to spawn the popup at an offset to get out of the way of one's thumbs. So on small screens, spawning from the top is more of than not the only viable option.

Keeping a consistent reading line

on small screens

I can see the appeal, on small screens, of – whenever spawning the popup above the lookup position – always hanging it from the top of the window (well, below the safe area insets, that is). And in cases where we have no choice but to spawn the popup below the lookup position, then we can hang it from the lookup position (plus the thumb clearance and line clearance).

But if we did hang it from the top of the window, although it would mean a consistent reading line, it would also mean further eye-travel. I wouldn't personally want to dart my eyes back and forth between the target sentence and the top of the window, even if the reading line were consistent (my iPhone 11 Pro has a rather tall screen).

on large screens

This is essentially a reiteration of what I've already mentioned for small screens. It's just that you more often have multiple options for where to lay out the popup, rather than only one viable option.

We can't solve all cases, but we can reverse the regression

  • If we locked the popup y-position between lookups, then the popup would always look odd when updating from a naturally small popup to a naturally large one.
  • If we hanged the popup from the top of the window, it means your eyes would have to jump a long distance between the text and the first definition.

So there's no good solution for improving the reading line of the popup when placing the popup above the lookup position.

However, it sounds like the old behaviour was simply to prefer placing the popup below the lookup position, minimising the eventualities in which the "above" position would be presented at all. So maybe the best we can do here is adjust the priority order to ensure that "below" is preferred instead of "above", at least for non-puck users.

As mentioned earlier, for puck users, preferring "below" is less viable because there's simply less space available below (due to thumbs and the iOS 15 safe area insets), and so more often than not, you'd get small popups appearing below and large popups appearing above, meaning that all the efforts towards keeping a consistent reading line would backfire.

How about that solution (the sentence in bold), then?

@nlgiles
Copy link
Author

nlgiles commented Sep 12, 2021

Thank you for giving so much thought to this. When I created this issue I hadn't thought about the complexities of small screen sizes, personally I don't use 10ten on a phone or tablet so I don't think I can give the best feedback about that.

Note that there is the option to turn on the j / k keys so you can force the popup to always be in, e.g. the bottom right, but that probably doesn't do exactly what you want.

Thank you for bringing this to my attention, I've tried using to force the pop-up into the top-left corner of my screen. You're correct that it's not my preferred solution but I didn't know about it before.

However, it sounds like the old behaviour was simply to prefer placing the popup below the lookup position, minimising the eventualities in which the "above" position would be presented at all. So maybe the best we can do here is adjust the priority order to ensure that "below" is preferred instead of "above", at least for non-puck users.

I've tried to gather up how I feel about this and summarize it. I'm going to start by assuming I'm always talking about horizontal lines of text as opposed to vertical lines as that's what I normally read, I don't know how my feelings would generalize to vertically oriented text.

I think that when the popup is being positioned relative to the text, its for the best that it is either entirely above or entirely below the text, and not to the left or right of the text. I think this was the the previous approach to popup placement. This approach maintains a constant reading line below horizontal text if popups appear below, but also can utilize the space above the the highlight when the highlight is in the bottom half of the screen.

With the current approach, popups are placed to the right or left of the highlight if the content in the popup would be too large to fit either above or below the highlight. When this happens the popup extends both above and below the highlight. This means the popup is larger than half of the height screen, often the entire height of the screen, at which point I feel like we've made a category error somewhere. A popup that covers more than three-fourths of your vertical screen space is more like it's own separate page at that point. Rather than having popups that large, I think it's at least acceptable and in many cases desirable to have the popup be limited to the space above or below the highlight, which is always at least half of the available vertical space. Then, for users that need to see the clipped bottom part of the popup there could be a feature to make popup content scrollable.

Perhaps there could also be an option to enable/disable the ability for popups to appear left/right of the highlight when they do not fit entirely in either the above or below space instead of clipping them.

For users on devices where that is not enough screen, placing the popup to the right or left of the highlight doesn't seem like it would fix the problem because if there is only enough screen space for a few lines of horizontal text, you still won't fit much of the popup on screen I think? Those users would probably want a solution where the popup always appears in a corner of the screen I think?

@nlgiles
Copy link
Author

nlgiles commented Sep 12, 2021

I'm cautious of speaking too severely on this issue because I'm not aware what drove the initial change to popup placement in the first place.

Ultimately, 10ten popups vary heavily in how many lines of text they contain. When highlighting a noun expressed with a small number of kanji characters and only a few meanings, there may only be a few definitions and the popup height is short. This popup will appear either above or below the highlight. When highlighting a hiragana phrase or a phrase with many connations, you get definitions for all substrings, sometimes including at the very bottom definitions for just the first hiragana character. This popup can be massive and take up the entire vertical height of the screen, moving the popup to the left or right of the highlight. The result is constant change of where the popup appears as you move between large and small popups, even when the highlight text is in the same area of the screen. Having my eyes jump back and forth from popups at the top of the screen to popups below the highlight, despite the highlights being in the same area of the screen, feels mentally confusing.

@birtles
Copy link
Member

birtles commented Sep 13, 2021

Thanks @shirakaba and @nlgiles for your very thoughtful consideration of the issues here.

Here are my takeaways:

  • About putting the popup on the left/right (for horizontal text):

    • On small screens it's almost never the case that we put the popup on left/right unless you have your phone (tablet?) in landscape mode
    • On desktop it's generally undesirable to position the popup on the left/right
  • About the preferred side to put the popup on:

    • For desktop users I think we prefer to put the popup below the text when possible. We already do that, however (specifically we prefer positioning below when not using the puck).
    • For phone users looking up words with the puck, the top of the screen is generally going to be preferable/more practical but using a fixed vertical position for the popup is possibly confusing and inconvenient.

Before making any suggestions, a little background on my understanding of that second point regarding where we are and how we got here:

  1. Previously the approach was: 'below' unless (a) it doesn't fit AND (b) there's more room above.

  2. Then we switched to an approach where we choose the best position based on which position has the greatest width or area (if the widths are the same). The idea is that we crop the height of the popup when it doesn't fit in the screen so the one with the biggest area is the one that has been cropped the least. In principle, that should give us roughly the same result, at least when deciding between above or below since they should have the same widths.

    However with approach (1) we would not constrain the height of the popup when it was in the 'below' position (since the user can always spin the mouse wheel to see the later, less relevant, results if they choose to). Once we applied this same behaviour to the width/area based approach, along with preferring 'below' to 'above' for mouse-based users, however, 'below' would almost always win and if you looked up text towards the bottom of the screen the popup would be mostly cut off and very hard to use. (We had code to try to detect which block position was not constrained but since 'below' was never constrained it wouldn't help.)

  3. In 6f604ee I tried to fix that so it wouldn't prefer necessarily the 'below' position if the popup didn't fit in the window.

    However, that still didn't fix the case where (in Slack in this case), if the popup was very tall, it wouldn't fit fully in the screen in either the 'above' or 'below' positions and so we would end up positioning the pop-up below, but on a page where we can't scroll hence making the popup unusable.

  4. In b591a1d I tried to fix that by detecting when the page is scrollable by adding some slightly complex code that I don't at all like but which at least meant the popup was usable.

My suggestions are:

  • Regarding the inline (typically left/right) positions I wonder if we have the following situation:
    • Desktop computer with a mouse reading horizontal text → The inline positions (left/right) aren't needed and are distracting.
    • Vertical text for the same situation? → Again, the inline positions (top/bottom) aren't needed
    • What about if you use the puck in the above situations? e.g. if you detach the keyboard from your Surface laptop? → There's still probably enough room to show enough of the popup that if the popup itself is scrollable it's still probably better to avoid the inline positions
    • Mobile phone in portrait orientation reading horizontal text with the puck? → Even if we allowed the inline direction popups (left/right) they're never going to fit so we're never going to choose them so it doesn't really matter.
    • Mobile phone in landscape orientation reading horizontal text with the puck → In this case, the horizontal positions could be quite helpful
    • Mobile phone in portrait orientation reading vertical text with the puck → In this case too, the vertical positions could be quite helpful.

So perhaps we could say something like, if the screen has more room in the inline direction than the block direction and the block extent is less than, say, 500 pixels, add the inline positions as candidates?

  • Regarding the above/below heuristic:
    • I think the original heuristic could still work. In all the years we've had it I can't recall a case where it was odd. It achieves similar results to what we have now but is much simpler (no trying to determine if the page is scrollable) etc.
    • We need to modify it, of course, to consider the safe area and clearance added for the puck etc. before deciding which side has more room.
    • We can also continue to keep the behavior where for non-puck users we prefer 'below' while for puck users we prefer 'above'.

Let me try turning that into a PR anyway and see what it looks like.

birtles added a commit that referenced this issue Sep 13, 2021
@birtles
Copy link
Member

birtles commented Sep 13, 2021

I ended up spending nearly all day on this but I think I came up with something that harmonises the old with the new over in #759. The key trick turned out to be changing when we revert the constrainHeight member. By doing that later, we can re-use a lot more code.

Still it's a pretty big patch because I also decided to generalise a fair bit of the positioning code. In my testing so far, however, it seems to do the right thing.

birtles added a commit that referenced this issue Sep 13, 2021
birtles added a commit that referenced this issue Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants