Tooltip: improve positioning in fallback case [devtools/devtools-frontend : main]
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
Related details
- Ergün Erdoğmuş
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ergün Erdoğmuş (Gerrit)
Ergün Erdoğmuş added 1 comment![]( "Open in Gerrit")
proposedRect.top = anchorRect.top - currentPopoverRect.height;This change would cause the tooltip to be rendered on top of the anchor, when the anchor is at the bottom of the viewport even though there is still space on top of the anchor. Can we keep offsetting by the anchor rect for that case?
See the screencast on the last comment here: https://g-issues.chromium.org/issues/40248266#comment44
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Auto-Submit | +1 |
| Commit-Queue | +1 |
1 comment
proposedRect.top = anchorRect.top - currentPopoverRect.height;This change would cause the tooltip to be rendered on top of the anchor, when the anchor is at the bottom of the viewport even though there is still space on top of the anchor. Can we keep offsetting by the anchor rect for that case?
See the screencast on the last comment here: https://g-issues.chromium.org/issues/40248266#comment44
Very nice catch! If the anchor is in a corner, the Tooltip should not even need the fallback position. `isInBounds` incorrectly did not allow equality. Fixed.
Related details
- Ergün Erdoğmuş
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 1 comment![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Auto-Submit | +1 |
1 comment
Related details
- Ergün Erdoğmuş
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ergün Erdoğmuş (Gerrit)
Ergün Erdoğmuş voted and added 4 comments![]( "Open in Gerrit")
Votes added by Ergün Erdoğmuş
| Code-Review | +1 |
4 comments
Picking the top or bottom based on which one has more space in that side sounds good to me!
bottomVerticalOutOfBoundsAmount =Instead of relying the side effects here, can we compute these values when needed?
(e.g. we first try the rectangles from the `uniqueOrder`, if they're all out of bounds, we calculate the `bottomVerticalOutOfBoundsAmount` and `topVerticalOutOfBoundsAmount` and decide what to do based on that.
Something like:
```
const bottomProposed = positioningUtils.bottomSpanRight({anchorRect, currentPopoverRect});
const bottomVerticalOutOfBounds =
Math.max(0, bottomProposed.top + currentPopoverRect.height - inspectorViewRect.bottom);
const topProposed = positioningUtils.topSpanRight({anchorRect, currentPopoverRect});
const topVerticalOutOfBounds = Math.max(0, inspectorViewRect.top - topProposed.top);const prefersBottom = bottomVerticalOutOfBounds <= topVerticalOutOfBounds;
```
We can also move this t a helper method I think.
const prefersBottom = bottomVerticalOutOfBoundsAmount <= topVerticalOutOfBoundsAmount;(nit): Instead of comparing the indexes of the options, we can iterate over the `uniqueOrder` array and pick the first one that fits the criteria:
```
const fallbackOption = uniqueOrder.find(option => {
if (prefersBottom) {
return option === PositionOption.BOTTOM_SPAN_LEFT || option === PositionOption.BOTTOM_SPAN_RIGHT;
}
return option === PositionOption.TOP_SPAN_LEFT || option === PositionOption.TOP_SPAN_RIGHT;
}) ?? PositionOption.TOP_SPAN_RIGHT;
```
const bottomVerticalOutOfBoundsAmount =(nit): Similarly, we can calculate the `bottomVerticalOutOfBoundsAmount` and `topVerticalOutOfBoundsAmount` when we need them.
Related details
- Wolfgang Beyer
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 3 comments![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Auto-Submit | +1 |
| Commit-Queue | +2 |
3 comments
Instead of relying the side effects here, can we compute these values when needed?
(e.g. we first try the rectangles from the `uniqueOrder`, if they're all out of bounds, we calculate the `bottomVerticalOutOfBoundsAmount` and `topVerticalOutOfBoundsAmount` and decide what to do based on that.Something like:
```
const bottomProposed = positioningUtils.bottomSpanRight({anchorRect, currentPopoverRect});
const bottomVerticalOutOfBounds =
Math.max(0, bottomProposed.top + currentPopoverRect.height - inspectorViewRect.bottom);const topProposed = positioningUtils.topSpanRight({anchorRect, currentPopoverRect});
const topVerticalOutOfBounds = Math.max(0, inspectorViewRect.top - topProposed.top);const prefersBottom = bottomVerticalOutOfBounds <= topVerticalOutOfBounds;
```We can also move this t a helper method I think.
Done
const prefersBottom = bottomVerticalOutOfBoundsAmount <= topVerticalOutOfBoundsAmount;(nit): Instead of comparing the indexes of the options, we can iterate over the `uniqueOrder` array and pick the first one that fits the criteria:
```
const fallbackOption = uniqueOrder.find(option => {
if (prefersBottom) {
return option === PositionOption.BOTTOM_SPAN_LEFT || option === PositionOption.BOTTOM_SPAN_RIGHT;
}
return option === PositionOption.TOP_SPAN_LEFT || option === PositionOption.TOP_SPAN_RIGHT;
}) ?? PositionOption.TOP_SPAN_RIGHT;
```
Done
(nit): Similarly, we can calculate the `bottomVerticalOutOfBoundsAmount` and `topVerticalOutOfBoundsAmount` when we need them.
`topVerticalOutOfBoundsAmount` is already only calculated if it's actually needed. So only `bottomVerticalOutOfBoundsAmount` is calculated speculatively. I prefer that to calculating the bottom proposed rectangle potentially twice.
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devtools-frontend LUCI CQ (Gerrit)
Devtools-frontend LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
5 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: front_end/ui/components/tooltips/Tooltip.ts
Insertions: 18, Deletions: 26.
The diff is too large to show. Please review the diff.
```
Change information
Tooltip: improve positioning in fallback case
Simple tooltips try to position themselves to the bottom center or top
center of their anchor. If neither of those options lie fully within
the viewport, pick a fallback position:
- Check whether top or bottom position is better, by picking the
option which is vertically out-of-bounds by a smaller amount.
- The tooltip position is then adjusted by moving it into the
viewport. This means the tooltip is allowed to cover its anchor in
the fallback case.
Rich Tooltips try to position themselves to the bottom/top left/right
of their anchor. If none of these 4 positions lie fully within the
viewport, a fallback position is calculated:
- Check whether top or bottom position is better, by picking the
option which is vertically out-of-bounds by a smaller amount.
- Choose left/right according to the preferred order.
- The tooltip position is then adjusted such that the tooltip stays in
the same corner of the viewport, but is moved into the viewport.
This means the tooltip is allowed to cover its anchor in the
fallback case.
- M front_end/ui/components/tooltips/Tooltip.test.ts
- M front_end/ui/components/tooltips/Tooltip.ts
Code-Review: +1 by Ergün Erdoğmuş
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |