Closed Bug 1454888 Opened 6 years ago Closed 6 years ago

Autocomplete postLabel overlaps with the scrollbar of the suggestions popup

Categories

(DevTools :: Inspector, defect, P3)

defect

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: jdescottes, Assigned: sarahchilds19)

References

Details

Attachments

(2 files, 3 obsolete files)

If there are enough values in the autocomplete popup to display a scrollbar, the rightmost part of the postlabel will be hidden by the scrollbar.

You can use the page in attachment to reproduce the issue.
Sarah, maybe you are interested in picking this bug?
Flags: needinfo?(sarahchilds19)
(In reply to Julian Descottes [:jdescottes][:julian] from comment #1)
> Sarah, maybe you are interested in picking this bug?

I am, I had thought I had noticed this before but I forgot to mention it sorry. I'll start on it right away. 

I'm not sure how to mark this bug as I'm working on it, so could you please do that? Thank you!
Flags: needinfo?(sarahchilds19)
Assignee: nobody → sarahchilds19
Status: NEW → ASSIGNED
I've run into an odd issue that I was hoping you may be able to explain since you know the autocomplete code better. 

Currently to determine if there is a scrollbar present I'm checking the if scrollHeight > clientHeight for the Tooltip.panel variable. This works perfectly for when I have entered "var(--" however, when first entering "var(-" the scroll and client heights are marked as 0 despite there visibly being a scrollbar. I've looked at all parts I could within the UpdateSize method, and have noticed that when the popup is opened through OpenPopup the heights only register as a value after all other code in the method has been executed. Despite this, if you try to update the size at the end of OpenPopup it still doesn't work as expected even though it now produces values for scroll/client height. 

Sorry, I'm a little lost with this bug, I can't figure out why it might be doing this for only the first - of a css variable.
Flags: needinfo?(jdescottes)
Indeed, the bug is more tricky than it seems. The reason you don't get any information the first time is that before we call this._tooltip.show() in openPopup, the list was never really rendered so it doesn't have dimensions yet. And as you've seen setting the size after is not good either because the container of the tooltip already "measured" the content and created a container with precise dimensions.

So our goal here would be to come up with a different layout that supports an element floating to the right as well as scrollbars. 

Right now we size the list with:
this._list.style.width = (this._maxLabelLength + 3) + "ch";  
The "+ 3" is here to compensate for scrollbars, but it doesn't handle items floating to the right.

Here if we look at the simplified markup for the popup we have:

  tooltip-container (with hardcoded width)
    tooltip-panel (max-height 20rem, this is the container that has the scrollbar)
      ul (this one has our calculated width in "ch" units)
        li
          span, span, span.postLabel

In common.css, if you move the max-height: 20rem and overflow-x: hidden from .devtools-autocomplete-popup (ie, tooltip-panel) to .devtools-autocomplete-listbox (ie, this._list) now the scrollbar will be taken into account and the postLabel should no longer go beneath the scrollbar. However it will look too compact. For this a solution is to bump the maxLabelLength if there is a postLabel (to be modified in get _maxLabelLength() {}).

I haven't fully tested it but it should go be the right direction.
Flags: needinfo?(jdescottes)
Attached patch 1454888.patch (obsolete) — Splinter Review
Taking Julian's suggestion I've changed moved the overflow/max-height properties to the listbox. I also noticed there seems to be a linux platform specific max-height property for the popup so I also changed that from popup to listbox. 

I wasn't certain if it's possible to have a mixture of autocomplete items that do/don't have post labels so I added a flag to the _maxLabelLength rather than checking a single item for a post label. I haven't encountered any mixed values, nor does there seem to be code for it, however I wanted to be certain it was in place if there happened to be. 

Thank you again Julian for your suggestion!
Attachment #8969679 - Flags: feedback?(jdescottes)
Attachment #8969679 - Flags: feedback?(gl)
Attachment #8969679 - Flags: feedback?(gl)
Comment on attachment 8969679 [details] [diff] [review]
1454888.patch

Review of attachment 8969679 [details] [diff] [review]:
-----------------------------------------------------------------

Looks in the good direction! We need a few more things to get this to display correctly when the toolbox is small.
If you apply the suggestions below we should be good.

::: devtools/client/shared/autocomplete-popup.js
@@ +267,3 @@
>        }
>  
>        max = Math.max(label.length, max);

(this is me being nitpicky more than a real issue)

In theory we should add the "+ 3" to the length here, before we compare it with the previous max.
It is not because there was a postLabel in the list of suggestions that the biggest suggestion necessarily had a postLabel.

::: devtools/client/themes/common.css
@@ +50,3 @@
>    box-shadow: 0 1px 0 hsla(209,29%,72%,.25) inset;
>    background-color: transparent;
>    border-radius: 3px;

With my two suggestions below, the "devtools-autocomplete-listbox" can start overflowing inside our container. We can set overflow: hidden; here to prevent this.

@@ +77,3 @@
>    border-width: 0px !important;
>    margin: 0;
>    padding: 2px;

Let's set box-sizing: border-box; to include this padding in size calculations.

@@ +77,5 @@
>    border-width: 0px !important;
>    margin: 0;
>    padding: 2px;
> +  overflow-x: hidden;
> +  max-height: 20rem;

When the toolbox is too small, the container might be smaller than 20rem. So we need to add `height: 100%;` here to make sure we respect the container's height as well.
Attachment #8969679 - Flags: feedback?(jdescottes) → feedback+
Comment on attachment 8969679 [details] [diff] [review]
1454888.patch

Review of attachment 8969679 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/themes/common.css
@@ +66,4 @@
>    margin: 0;
>  }
>  
> +:root[platform="linux"] .devtools-autocomplete-listbox {

Nice catch updating this rule!
Attached patch 1454888.patch (obsolete) — Splinter Review
I've made the suggested changes. I tested this with the other html file attached to this bug and didn't have any issues, so let me know if the adjustments have fixed the issues you mentioned please.
Attachment #8969679 - Attachment is obsolete: true
Attachment #8971662 - Flags: feedback?(jdescottes)
Comment on attachment 8971662 [details] [diff] [review]
1454888.patch

Review of attachment 8971662 [details] [diff] [review]:
-----------------------------------------------------------------

Patch works nicely, thanks Sarah! 
Small issue with the label length calculation, but with the suggested fix (or anything similar) we should be able to land this soon.

Feel free to directly flag me for "review?" rather than "feedback?" here.

::: devtools/client/shared/autocomplete-popup.js
@@ +262,4 @@
>  
>        if (postLabel) {
>          label += postLabel;
> +        label += 4; // Increases the size to account for scrollbars

label is a string so we can't really do that. For instance if label is "mylabel", label += 4 will be "mylabel4". 

Here we can do:
  const length = label.length + (postLabel ? 3 : 0);
  max = Math.max(length, max);
Attachment #8971662 - Flags: feedback?(jdescottes) → review+
Attached patch 1454888.patch (obsolete) — Splinter Review
Thank you for noticing that, I think I must have been half asleep when I was fixing that up sorry. I've attached the patch with your suggested fix.
Attachment #8971662 - Attachment is obsolete: true
Attachment #8972393 - Flags: review?(jdescottes)
Thanks! The last patch is identical to the previous one, probably a mistake.
Can you upload the proper patch? If it's a bother I can amend your patch, but I would prefer to land yours if possible!
Flags: needinfo?(sarahchilds19)
Attached patch 1454888.patchSplinter Review
Wow, I am so sorry about that. I forgot to refresh the patch before I uploaded it.
Attachment #8972393 - Attachment is obsolete: true
Attachment #8972393 - Flags: review?(jdescottes)
Flags: needinfo?(sarahchilds19)
Attachment #8972446 - Flags: review?(jdescottes)
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/288cf7227697
Autocomplete postLabel overlaps with the scrollbar of the suggestions popup. r=jdescottes
Comment on attachment 8972446 [details] [diff] [review]
1454888.patch

Review of attachment 8972446 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks a lot for fixing this Sarah!
This fix should now ship with Firefox 61.
Attachment #8972446 - Flags: review?(jdescottes) → review+
https://hg.mozilla.org/mozilla-central/rev/288cf7227697
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.