1624238 - browserAction.setTitle() has problems with newline characters

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[kernp25]

Attachment: test.zip

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:74.0) Gecko/20100101 Firefox/74.0

Steps to reproduce:

1. Load the attached add-on

Actual results:

It will show: "First line.Second line."

See Image.

Expected results:

It should just show: "First line."

But the tooltip should still display:
First line.
Second line.

Comment 1

[kernp25]

Attachment: without-tooltip.png

Comment 2

[kernp25]

Attachment: with-tooltip.png

Comment 3

[kernp25]

What do you think?

The use case is that, in my other add-on i want to display important information in the tooltip.

But it looks bad when the button is in the popup panel (moved by user).

Comment 4

[kernp25]

Expected results:

In the popup panel:
My add-on

In the tooltip:
My add-on
Important information

Comment 5

[kernp25]

Or other option:
The popup panel should just show the add-on name.
And the tooltip should display what the add-on set with browserAction.setTitle().

Results would be:

In the popup panel:
My add-on

In the tooltip:
Important information

Comment 6

[kernp25]

Attachment: test.zip

Comment 7

[kernp25]

Attachment: image_1.png

Comment 8

[kernp25]

Attachment: image_2.png

Comment 9

[kernp25]

Attachment: image_3.png

How it looks like in the popup window.

Comment 10

[Rob Wu [:robwu]]

The bug report is unclear. Could you explain what you're expecting and what happens instead, for a given STR?

Comment 11

[kernp25]

Tooltips does support line breaks but when using it with browserAction.setTitle() it will look strange in the menu panel.

See image from: https://bugzilla.mozilla.org/show_bug.cgi?id=1624238#c9

Comment 12

[kernp25]

Attachment: RjY9k5HGdj.png

How it looks like with only 1 line.

Comment 13

[kernp25]

The bug is that, the menu panel is broken when the title contains line breaks.

Comment 14

[kernp25]

Expected results:
browserAction.setTitle() should support line breaks, but without breaking the menu panel.

Comment 15

[Rob Wu [:robwu]]

Attachment: repro-nightly-76-20200325093457.ogv

When I load the extension, reveal the add-on button and click on it to open the popup, I see that the panel is loaded and that the heading is shifted to the left. This looks wrong. Are you referring to this bug?

I tried to debug this with the Browser Toolbox, and disabling popup auto-hide. After inspecting the affected area, I see the following markup of interest:

<panelview ...
  id="PanelUI-webext-adf8405353d0cd89d86893ce618ef0f0acf90f88_temporary-addon-browser-action-view" ...
  style="max-width: 348px; min-width: 348px;" ...
>
  <box class="panel-header">
    <toolbarbutton ...>... </toolbarbutton>
    <label>
      <span xmlns="http://www.w3.org/1999/xhtml">
Firstttttttttttttttt line.
Secondddddddddddddd line.
Enddddddddddddd line.
      </span>
    </label>
  </box>
  ...
</panelview>

The <panelview> element's dimensions is constrained by the max-width property, which appears to be set here: https://searchfox.org/mozilla-central/rev/b712398b7fae54ef377a558d6f16dede7a7f8530/browser/components/customizableui/PanelMultiView.jsm#718

When the inline style is not set, it falls back to the max-width (=29em) from the stylesheet at https://searchfox.org/mozilla-central/rev/b712398b7fae54ef377a558d6f16dede7a7f8530/browser/themes/shared/customizableui/panelUI.inc.css#374-375

The visual bug disappears when I remove overflow:hidden from the panelview - https://searchfox.org/mozilla-central/rev/b712398b7fae54ef377a558d6f16dede7a7f8530/browser/themes/shared/customizableui/panelUI.inc.css#349-351

I don't know why though...

Comment 16

[kernp25]

(In reply to Rob Wu [:robwu] from comment #15)

When I load the extension, reveal the add-on button and click on it to open the popup, I see that the panel is loaded and that the heading is shifted to the left. This looks wrong. Are you referring to this bug?

I only tested it in Firefox 74.0. But it must be the same bug, as mentioned in Comment 13.

Comment 17

[Rob Wu [:robwu]]

Gijs, see comment 15. Do you have an idea what's going on?

I'm also wondering why removing overflow:hidden fixes the bug (it was introduced in bug 1282189).
(I haven't checked for side effects, i.e. the impact of its removal in other cases.)

Comment 18

[kernp25]

What do you think of the idea from Comment 5?

Comment 19

[Rob Wu [:robwu]]

I'm not against showing the add-on name instead of the tooltip, but it seems that the heading is derived from the title: https://searchfox.org/mozilla-central/rev/b712398b7fae54ef377a558d6f16dede7a7f8530/browser/components/customizableui/PanelMultiView.jsm#714

The implementation can change if really needed, but I'd like to avoid too much special-casing if possible.

Comment 20

[kernp25]

(In reply to Rob Wu [:robwu] from comment #19)

I'm not against showing the add-on name instead of the tooltip, but it seems that the heading is derived from the title: https://searchfox.org/mozilla-central/rev/b712398b7fae54ef377a558d6f16dede7a7f8530/browser/components/customizableui/PanelMultiView.jsm#714

The implementation can change if really needed, but I'd like to avoid too much special-casing if possible.

But you can see, how bad it looks like in the image from Comment 7.

Comment 21

[kernp25]

My add-on does this:
test@gmail : 5
[e-mail] : 10

It is a email notifier and it would like to display the email address with the email count.

It would display in the menu panel like this:
test@gmail : [e-mail] : 10

Comment 22

[kernp25]

Attachment: IMw87Bkixx.png

Comment 23

[kernp25]

Attachment: 9w3PCp83bc.png

How it looks like in Google Chrome.

Comment 24

[:Gijs (out for now; he/him)]

(In reply to Rob Wu [:robwu] from comment #17)

Gijs, see comment 15. Do you have an idea what's going on?

Not off-hand. It looks vaguely like something that's supposed to be in a subtree under an element with overflow:hidden and thus invisible (having been "slid out of view" after a subview animation is not, in fact, invisible. I don't know why that would be. Equally, the top bit looks cut-off mid-item, which almost looks like a graphics issue of some kind - unless the text is forcing the element to be wider than its container, and the overflow:hidden means the left side gets cut off or something?

I'm also wondering why removing overflow:hidden fixes the bug (it was introduced in bug 1282189).
(I haven't checked for side effects, i.e. the impact of its removal in other cases.)

Generally, I'm confused by the direction this bug is going in. It seems the request in comment #0 was relatively simple: make the title a single-line string (so strip anything after the first newline, if any), and display the full thing in the tooltip. It seems like we could do that and that'd also fix the display issue you're seeing? Is there some reason we don't want to pursue that?

Comment 25

[Rob Wu [:robwu]]

(In reply to :Gijs (he/him) from comment #24)

Equally, the top bit looks cut-off mid-item, which almost looks like a graphics issue of some kind - unless the text is forcing the element to be wider than its container, and the overflow:hidden means the left side gets cut off or something?

Which top? In the video in comment 15, the top-right part is a rendering issue (demonstrated by the inability to interact with it by the mouse). Often it is just blank.
The line that ends midway is normal, it is the border-bottom of the .panel-header element.

I'm also wondering why removing overflow:hidden fixes the bug (it was introduced in bug 1282189).
(I haven't checked for side effects, i.e. the impact of its removal in other cases.)

Generally, I'm confused by the direction this bug is going in. It seems the request in comment #0 was relatively simple: make the title a single-line string (so strip anything after the first newline, if any), and display the full thing in the tooltip.

The bug was not clear to me and it's a moving target (see e.g. comment 5). I'm more concerned about the rendering bug though, and am looking into it to see if we can file a follow-up bug to fix whatever underlying bug is there.
The rendering bug is not limited to line breaks; I can reproduce the same issue if I load an extension with a very long title:

{
    "name": "Long button title",
    "version": "1",
    "manifest_version": 2,
    "browser_action": {
        "default_area": "menupanel",
        "default_popup": "manifest.json",
        "default_title": "abcdefghijklmnopqrstuvwxyz0123456789_abcdefghijklmnopqrstuvwxyz0123456789_abcdefghijklmnopqrstuvwxyz0123456789"
    }
}

It seems like we could do that and that'd also fix the display issue you're seeing? Is there some reason we don't want to pursue that?

I tried white-space:nowrap, and although that cut the title, the rendering issue (=a big gap appearing at the right) did not disappear.

If we can't figure out the bug here, I can just file a new bug (in which product/component?) and let the triage owner handle it.
Then this bug can return to kernp's request (which is either in comment 0 or comment 5).

Comment 26

[Rob Wu [:robwu]]

See previous comment

Comment 27

[:Gijs (out for now; he/him)]

(In reply to Rob Wu [:robwu] from comment #25)

If we can't figure out the bug here, I can just file a new bug (in which product/component?) and let the triage owner handle it.
Then this bug can return to kernp's request (which is either in comment 0 or comment 5).

That seems like a good idea. I take it just text-overflow: ellipsis on the title doesn't fix it? If not, file a new bug in Firefox :: Toolbars & Customization and ping me there, please. Thank you!

Comment 28

[Rob Wu [:robwu]]

I added text-overflow:ellipsis on every element from the <span> element and upwards, but didn't see any difference in behavior.

I've filed bug 1625600 and included a bit of interaction with the devtools in the video, please take a look when you have some time.

Comment 29

[kernp25]

But i think, if you fix bug 1625600, it would also work for me, if you replace the line with a space. Then it would look like this:
test@gmail : 5 [e-mail] : 10

And not:
test@gmail : [e-mail] : 10

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 31

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 32

[:Gijs (out for now; he/him)]

FWIW, I think bug 1574196 made the header wrap in the protections/identity panel cases... but only added styling for those controls, while adding a <span> (and moving away from label.value) everywhere.

Nihanth, can you check if that caused a change for webextensions? I would expect so...

Comment 33

[(inactive) Nihanth Subramanya [:nhnt11]]

I think Gijs is right. Taking this.

Comment 34

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Bug 1624238 - Enable PanelMultiView header text to span multiple lines. r=Gijs

Comment 35

[(inactive) Nihanth Subramanya [:nhnt11]]

Attachment: Screenshot with patch

Any reason we can't just add a white-space: pre to the span? This seems to fix the issue for me without messing with existing PanelMultiView consumers. It doesn't affect the text in the menulist item though. (Patch is up as well to demonstrate)

Comment 36

[(inactive) Nihanth Subramanya [:nhnt11]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51826a845b1e8aefba35a52122f841678f3393d

Comment 37

[(inactive) Nihanth Subramanya [:nhnt11]]

Bad try push. Re-pushed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=65d67bc2eabce4007b3be8f29c3c1cca018d53a4

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e0669c56f556
Enable PanelMultiView header text to span multiple lines. r=Gijs,robwu

Comment 39

[Stefan Hindli [:stefan_hindli]]

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