900415 - Should be able to open the autocompletion popup at an offset from the anchor node.

Status: VERIFIED FIXED

(DevTools :: General, defect)

Description

[Girish Sharma [:Optimizer]]

This bug will take care of the following things :

- Provide API to open the popup at an offset
- Open the popup at correct offset in markup view
- Open the popup at correct offset in Web Console

Comment 1

[Girish Sharma [:Optimizer]]

Attachment: patch v0.1

@Victor, does it make things better on your Mac ?

Comment 2

[Victor Porof [:vporof][:vp]]

Comment on attachment 786826 [details] [diff] [review]
patch v0.1

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

This is not particularly better, the popup is still quite jumpy, both horizontally and vertically. And now the inner list is squeezed with no margins.
I'm incredibly worried about the massive amount of hackery that keeps being injected into this autocompletion popup just to fix UI annoyances. From what I've seen, none of the issues happen for CSS autocompletion. Why can't we go that route then and have a fixed width?

Comment 3

[Girish Sharma [:Optimizer]]

(In reply to Victor Porof [:vp] from comment #2)
> Comment on attachment 786826 [details] [diff] [review]
> patch v0.1
> 
> Review of attachment 786826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is not particularly better, the popup is still quite jumpy, both
> horizontally and vertically. And now the inner list is squeezed with no
> margins.
I am surprised. How is your Mac 10.8 so different than mine that I do not see any jumping with this patch, but you see both horizontal and vertical. Can I please request you to post a screencast ? Thanks.

The list is not squeezed. Its just that the margin : 4px has been converted to padding : 4px for the rich list items for certain reasons.

> I'm incredibly worried about the massive amount of hackery that keeps being
> injected into this autocompletion popup just to fix UI annoyances. From what
> I've seen, none of the issues happen for CSS autocompletion. Why can't we go
> that route then and have a fixed width?

CSS has a limit on the width of the elements (maximum is -moz-animation-function-property), but JS does not. If we choose a very big width, then it will remain half empty for most of the time, but if we choose a very small width, then scrollbars will appear making it look messy.

Comment 4

[Mihai Sucan [:msucan]]

I tend to agree with Victor here, we should try a fixed width popup for the webconsole as well. I believe I have previously suggested this as well.

The hackery we need for the popup to have dynamic width/height is ever-changing for various reasons. I'd like us to avoid it entirely.

Comment 5

[Girish Sharma [:Optimizer]]

I agree for fixed width. But I will still like to have a screencast if possible, and if it is not that much trouble for Victor.

Comment 6

[Victor Porof [:vporof][:vp]]

(In reply to Girish Sharma [:Optimizer] from comment #5)
> I agree for fixed width. But I will still like to have a screencast if
> possible, and if it is not that much trouble for Victor.

It's pretty much the same thing as shown in the screencast in bug 857441.

Comment 7

[Girish Sharma [:Optimizer]]

Attachment: patch v0.2

This should work.
It looks like this on my mac : http://youtu.be/BZyEvApWUIo

Comment 8

[Girish Sharma [:Optimizer]]

try push : https://tbpl.mozilla.org/?tree=Try&rev=f8bdf63e995d

Comment 9

[Mihai Sucan [:msucan]]

Comment on attachment 790922 [details] [diff] [review]
patch v0.2

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

Tested on MacOSX: the popup always has scrollbars, even when they are not needed. Is there any reason for that?

Comment 10

[Girish Sharma [:Optimizer]]

Were your scrollbars overlaying scrollbars ?

Also, they should not be shown if not needed. I am not sure that I saw them on my machine. will retest again.

Comment 11

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #10)
> Were your scrollbars overlaying scrollbars ?

No. Normal scrollbars.

Comment 12

[Girish Sharma [:Optimizer]]

So I think the issue is happening because the way I detect overlaying scrollbars. If only I have a foolproof way to know if the scrollbars are overlaying or not, everything else should be fine.

Mihai, do you know of such a way ?

Comment 13

[Mihai Sucan [:msucan]]

(In reply to Girish Sharma [:Optimizer] from comment #12)
> So I think the issue is happening because the way I detect overlaying
> scrollbars. If only I have a foolproof way to know if the scrollbars are
> overlaying or not, everything else should be fine.
> 
> Mihai, do you know of such a way ?

Unfortunately, I do not know of a way to detect overlaying scrollbars. Try to ping Neil Deakin or Neil Rashbrook - they might know.

Comment 14

[Girish Sharma [:Optimizer]]

Attachment: patch v0.3

Mihai, I think this should work properly in your osx now as this is the correct way to check for floating scrollbars (thanks Paul) (atleast from what I have tested, it works for me).

Comment 15

[Mihai Sucan [:msucan]]

Comment on attachment 819594 [details] [diff] [review]
patch v0.3

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

Thanks for the updated patch.

The changes you do for sizing the popup make me uneasy. That code is getting too hacky.

Screenshot: http://img.i7m.de/show/khtfr.png

I have weird scrolllbars since weeks ago, with and without this patch - Ubuntu 12.04. Anything we can do? I'd like a simplification of all the popup sizing code.

::: browser/devtools/shared/autocomplete-popup.js
@@ +127,5 @@
>     *
>     * @param nsIDOMNode aAnchor
>     *        Optional node to anchor the panel to.
>     */
> +  openPopup: function AP_openPopup(aAnchor, aXOffset = 0, aYOffset = 0)

Please add the new parameters in the jsdoc.

@@ +229,5 @@
> +    // If we have to resize the popup, add a margin equivalent of the scrollbar's
> +    // width so that we do not see a flickering.
> +    if (!this.fixedWidth &&
> +        (this.overflowing || aItems.length > this.itemCount)) {
> +      this._list.style.marginRight = this._scrollbarWidth + "px";

Why do you add this piece of code here? I understand you are trying to avoid flickering, but I worry this is going to cause other issues. How can I reproduce the flickering? I want to see the effect of this code.

@@ +282,5 @@
>      }
> +    else {
> +      this.overflowing = false;
> +    }
> +    this._list.style.marginRight = "0";

Please explain these changes, in an otherwise confusing function.

@@ +506,5 @@
>     * @private
>     */
>    get _scrollbarWidth()
>    {
> +    if (this.__scrollbarWidth !== undefined) {

Please add __scrollbarWidth to the prototype of the object and set it to null.

::: browser/devtools/shared/inplace-editor.js
@@ +1078,5 @@
>        }
>  
> +      // Calculate the offset for the popup to be opened.
> +      let x = (this.input.selectionStart - startCheckQuery.length) *
> +              this.inputCharWidth;

You could put this new |x| in the |if (finalList.length > 1) { ... }| code block.

::: browser/devtools/webconsole/webconsole.js
@@ +694,5 @@
> +    this._updateCharSize();
> +  },
> +
> +  /**
> +   * recalculates the width and height of a single character of the input box.

s/recaculates/Calculate/

@@ +713,5 @@
> +    copyTextStyles(this.inputNode, tempLabel);
> +    tempLabel.textContent = "x";
> +    doc.documentElement.appendChild(tempLabel);
> +    this.inputCharWidth = tempLabel.offsetWidth;
> +    this.inputCharHeight = tempLabel.offsetHeight;

Please make these properties private and add them to the prototype, with a comment.

@@ +715,5 @@
> +    doc.documentElement.appendChild(tempLabel);
> +    this.inputCharWidth = tempLabel.offsetWidth;
> +    this.inputCharHeight = tempLabel.offsetHeight;
> +    tempLabel.parentNode.removeChild(tempLabel);
> +    tempLabel = null;

nullifying a local variable is not needed.

@@ +718,5 @@
> +    tempLabel.parentNode.removeChild(tempLabel);
> +    tempLabel = null;
> +    // Calculate the width of the chevron placed at the beginning of the input
> +    // box. Remove 4 more pixels to accomodate the padding of the popup.
> +    this.chevronWidth = +doc.defaultView.getComputedStyle(this.inputNode)

... private property and add it to the prototype.

@@ +4315,5 @@
>  
>      if (items.length > 1 && !popup.isOpen) {
> +      let x = (this.inputNode.selectionStart - lastPart.length) *
> +              this.hud.inputCharWidth;
> +      let y = this.inputNode.value.slice(0, this.inputNode.selectionStart)

The |y| variable is not used. Also, I prefer that the popup location does not change on the y-axis.

Comment 16

[Girish Sharma [:Optimizer]]

Attachment: patch v0.4

Removed the further addition of hacks for the popup.
The consensus is that the issues occur only in dynamic width popup, where we need to update the width of the popup. Only web console uses the dynamic width popup and soon the implementation of autocomplete in web console will change once the JSTerm lands. Thus I am not trying to add more hacks to partially fix a thing that is soon going to go away.

Comment 17

[Girish Sharma [:Optimizer]]

(In reply to Mihai Sucan [:msucan] from comment #15)
> >  
> >      if (items.length > 1 && !popup.isOpen) {
> > +      let x = (this.inputNode.selectionStart - lastPart.length) *
> > +              this.hud.inputCharWidth;
> > +      let y = this.inputNode.value.slice(0, this.inputNode.selectionStart)
> 
> The |y| variable is not used. Also, I prefer that the popup location does
> not change on the y-axis.

It is used to detect the number of lines in the input box. Because we will have to reduce the x offset by that much amount.
And no, I am not changing the vertical position of the popup.

Comment 18

[Girish Sharma [:Optimizer]]

Attachment: patch v0.5

Forgot to change the undefined check to null check since I added the __scrollbarWidth to prototype with null as initial value.

Comment 19

[Mihai Sucan [:msucan]]

Comment on attachment 822382 [details] [diff] [review]
patch v0.5

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

Patch looks good. r+ with the comments below addressed. Thank you!

Please make sure you get green try pushes.

::: browser/devtools/webconsole/webconsole.js
@@ +729,5 @@
> +   */
> +  _updateCharSize: function WCF__updateCharSize()
> +  {
> +    let doc = this.document;
> +    let tempLabel = doc.createElementNS("http://www.w3.org/1999/xhtml", "span");

Use the XHTML_NS constant.

@@ +4383,5 @@
> +      let y = this.inputNode.value.slice(0, this.inputNode.selectionStart)
> +                  .split("\n").length - 1;
> +      if (y > 0) {
> +        x -= (this.inputNode.value
> +                  .lastIndexOf("\n", this.inputNode.selectionStart) + y) *

This is confusing. Why not directly get the offset from the \n character to the caret?

Like:

str = inputValue.substr(0, selectionStart);
pos = str.lastIndexOf("\n");
offset = str.length - pos;
x = offset * charWidth;

Am I missing something?

@@ +5178,5 @@
>    return gSequenceId.n++;
>  }
>  gSequenceId.n = 0;
>  
> +function copyTextStyles(aFrom, aTo)

Move this function to the Utils object.

Comment 20

[Girish Sharma [:Optimizer]]

Attachment: patch v0.6

Addressed review comments.
Used the method proposed by Mihai to calculate offset. Just that the popup needs to be opened at the starting of the word, not at caret position, thus lastPart.length subtraction.


Carrying forward r+

try : https://tbpl.mozilla.org/?tree=Try&rev=c3c393bcb222

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a3ea4025d340

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3ea4025d340

Comment 23

[Petruta Horea [:phorea], Desktop QA]

While verifying the issue on latest Aurora (20140203004003) under Win7, 64-bit, I noticed that writing "document.all.__d" gives two options (defineGetter and defineSetter). Adding "e" (document.all.__de) deletes the two options. Encountered with other strings as well.

Is this expected? Thank you

Comment 24

[Girish Sharma [:Optimizer]]

That is not supposed to happen. In fact, on beta, I do not get any suggestions for "document.all.".

In any case, please open a new bug, as it is out of scope of this bug. This bug only handles the proper horizontal alignment of the popup.

Comment 25

[Petruta Horea [:phorea], Desktop QA]

Thank you Girish!

Verified that the autocompletion popup is correctly alligned on latest Aurora 28.0a2 under Win 7 64-bit, Ubuntu 32-bit and Mac OSX 10.9.

I logged bug 967468 for the issue described in Comment 23.