Don't use a XUL label for displaying the selected filename in the filepicker UI

RESOLVED FIXED

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
4 days ago

People

(Reporter: bgrins, Unassigned)

Tracking

(Blocks 3 bugs)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [overhead:noted])

Attachments

(4 attachments, 3 obsolete attachments)

(Reporter)

Description

a year ago
Using a <xul:label> [0] means that the label XBL binding runs in-content, and that we have XUL text CSS selectors (like label) inside of minimal-xul.css - although I believe the CSS could move into xul.css ahead of changing the implementation here, given XULElementsRulesInMinimalXULSheet [1].  There are also some CSS rules targeting `input[type="file"] > xul|label` in forms.css [2].

[0] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/layout/forms/nsFileControlFrame.cpp#144
[1] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/dom/xul/nsXULElement.cpp#683
[3] https://searchfox.org/mozilla-central/rev/3abf6fa7e2a6d9a7bfb88796141b0f012e68c2db/layout/style/res/forms.css#504
(Reporter)

Updated

a year ago
Blocks: 1446831
(Reporter)

Comment 1

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #0)
> we have XUL text CSS selectors (like label) inside of
> minimal-xul.css - although I believe the CSS could move into xul.css ahead
> of changing the implementation here, given XULElementsRulesInMinimalXULSheet
> [1].

Correction: we *will* need to change the implementation here before moving these out of minimal-xul.css (Bug 1446831), since we won't ever be loading xul.css in the content document after Bug 1444193.
Comment on attachment 8960044 [details]
Bug 1446830 - Alternate implementation that uses HTML label with end crop instead of center

https://reviewboard.mozilla.org/r/228808/#review234502

::: layout/forms/nsFileControlFrame.cpp:476
(Diff revision 1)
>  #endif
>  
>  void
>  nsFileControlFrame::UpdateDisplayedValue(const nsAString& aValue, bool aNotify)
>  {
> -  mTextContent->SetAttr(kNameSpaceID_None, nsGkAtoms::value, aValue, aNotify);
> +  // XXX: XUL label allows crop: center. Can we use CSS to get similar behavior?

Nice find.

for <xul:label crop=center> can we could use CSS text-overflow to do create similar visuals?
(Reporter)

Comment 5

a year ago
A test document with widths set on both the file input and the parent div of the file inputs. The only case I can trigger the crop=center behavior is when the width is set on the file input and it's not too small (I start to see the ellipses around 175px).
(Reporter)

Comment 6

a year ago
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #4)
> for <xul:label crop=center> can we could use CSS text-overflow to do create
> similar visuals?

Unfortunately middle cropping isn't part of the standard (see also bug 740910). We can probably make cropping at the end work with CSS, although in that case you wouldn't see the file extension.
See Also: → 740910
(Reporter)

Comment 7

a year ago
(In reply to Brian Grinstead [:bgrins] from comment #6)
> (In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment
> #4)
> > for <xul:label crop=center> can we could use CSS text-overflow to do create
> > similar visuals?
> 
> Unfortunately middle cropping isn't part of the standard (see also bug
> 740910). We can probably make cropping at the end work with CSS, although in
> that case you wouldn't see the file extension.

Neil, do you have any ideas on how we could accomplish a crop="middle" effect for an HTML label inside the file picker's anon content? Right now I'm just setting text content for the label (see Part 1) but I guess we could render out the DOM however we wanted.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 8

a year ago
Oh also: crop=center in general seems pretty lightly used https://searchfox.org/mozilla-central/search?q=crop%3D%22center%22&path=. Maybe we should come up with a way to do 'filename cropping' for this case and for downloads instead of supporting generic center cropping behavior on xul / html labels
(Reporter)

Comment 9

a year ago
FWIW: On the test page with a long filename (really-long-file-name-123456789-abcdefghijklmnopqrstuvwxyz.txt) I see some odd behavior where the center crop doesn't show the end of the string so you can't see the file extension.
(Reporter)

Updated

a year ago
Blocks: war-on-xbl

Comment 10

a year ago
There isn't any other text displaying frame that crops like that.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 11

a year ago
Suggestion from roc: maybe you could split the filename into two span elements, one containing the directory and one the file name itself, and make the directory element text-overflow:ellipsis, then put both elements in a flexbox so that the filename gets its intrinsic width and the directory element gets whatever is left over. Include a min-width on the directory part so it won't collapse down to 0.
(Reporter)

Comment 12

a year ago
Suggestion from dholbert: you could build a chrome-only "text-overflow: middle" (and could begin by prototyping the behavior you want in text-overflow: ellipsis).
(Reporter)

Updated

a year ago
See Also: → 1421382
(Reporter)

Comment 13

a year ago
Found a couple of potential CSS solutions for the center crop behavior by splitting the text into two siblings: https://codepen.io/anon/pen/LdWLem and https://codepen.io/anon/pen/ZxeyZJ. Going to spend some more time looking into it.
(In reply to Brian Grinstead [:bgrins] from comment #13)
> Found a couple of potential CSS solutions for the center crop behavior by
> splitting the text into two siblings: https://codepen.io/anon/pen/LdWLem and
> https://codepen.io/anon/pen/ZxeyZJ. Going to spend some more time looking
> into it.

They look ... hacky. I would worry the accessiblity tree these markup results.
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8960045 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 20

a year ago
Ehsan, we discussed previously about how the current file picker form control forces ltr on the label that displays the filename. Would you be able to check this iteration of the patch and let me know how it compares with the status quo?

For whatever reason, when I set [dir=auto] on the NAC label it doesn't change direction based on the filename text, so instead this just inherits directionality from the input. Which means if we have an rtl input with ltr filename text it renders with the wrong directionality (basically like what happens always today with rtl filename). I'm not sure if this is better or worse than current behavior.

I set up a test page at https://garnet-wine.glitch.me/ that shows a few different configurations and makes it easier to trigger overflows.
Flags: needinfo?(ehsan)

Comment 21

a year ago
Sure, any chance you could make a try build please?  Thanks!
(Reporter)

Comment 22

a year ago
(In reply to :Ehsan Akhgari from comment #21)
> Sure, any chance you could make a try build please?  Thanks!

Yes, here it is: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df3b5be6ef96ac52e011eb06abb7d3a9635e6ee9

Comment 23

a year ago
I think this is worse than the current behavior.  :-(  Note that the text on the button comes from the UI language, so this would mean the "Browse..." text would look broken on all LTR builds when viewing an RTL page.

If you can file a bug about [dir=auto] not working on NAC, I think we can make it work.  I suspect there is something broken in the code in DirectionalityUtils.cpp that doesn't account for native anonymous content properly somehow...  It doesn't seem worth sacrificing a user-facing behavior due to a bug!
Flags: needinfo?(ehsan)
(Reporter)

Comment 24

a year ago
(In reply to :Ehsan Akhgari from comment #23)
> I think this is worse than the current behavior.  :-(  Note that the text on
> the button comes from the UI language, so this would mean the "Browse..."
> text would look broken on all LTR builds when viewing an RTL page.
> 
> If you can file a bug about [dir=auto] not working on NAC, I think we can
> make it work.  I suspect there is something broken in the code in
> DirectionalityUtils.cpp that doesn't account for native anonymous content
> properly somehow...  It doesn't seem worth sacrificing a user-facing
> behavior due to a bug!

OK, I'll file a bug for it. If fixing it turns out to not be viable we could always restore the forced ltr behavior we currently use but ofc it'd be nicer to fix this up while we're here.

Comment 25

a year ago
Another workaround could also be dropping the dots at the end of these strings, since doing that would mask the directionality of the strings.  ;-)  But if you decide to go down that path please reach out to the l10n folks and consult with them, since ensuring these types of hacks work properly across 80+ localizations is difficult...
(Reporter)

Updated

a year ago
Depends on: 1451576
The use of *nsIContent->SetText() need to be changed to *nsIContent->AsText()->SetText(). The method is removed in bug 1449404.
Comment hidden (mozreview-request)

Updated

a year ago
Priority: -- → P5
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
(Reporter)

Updated

a year ago
Blocks: 1457628

Updated

10 months ago
Whiteboard: [overhead:noted]
Comment hidden (mozreview-request)
(Reporter)

Comment 30

10 months ago
I've pushed up a simpler version that replaces the xul:label with an html:label and a bit of flexbox to get text-overflow to work. The main difference is that instead of cropping the string in the middle (foo…baz.txt) it crops at the end (foobar…).

I think it's possible to replicate the middle crop somehow but it's quite a bit of work - middle-crop isn't a thing in CSS/HTML so we'd need to implement something specific for this use-case.

Given that this is blocking some XBL removal progress and a small content process memory win, I'd like to at least check to see if this is something that we'd be OK with landing as-is even though we'd lose the middle-crop (which lets you see both the beginning and end of a file name). Shorlander, would you be able to give an opinion or redirect this?

A few notes:
1) I've put together a test page at https://garnet-wine.glitch.me/ to see the input in various configurations. The attached screenshot shows what it looks like to select `really-long-file-name-123456789-abcdefghijklmnopqrstuvwxyz.txt` with a constrained width both with and without the patch applied.
2) The current XUL middle crop doesn't always work very well - you can see in the screenshot that you don't see the actual end of the string.
3) The full URL can be seen on hover with the tooltip text in both cases
4) The crop only happens when there's not enough space (usually when a width is set). On cases like bugzilla's file input or `data:text/html,<input type="file">` you can have a pretty huge file name before it starts to crop
Flags: needinfo?(shorlander)
(Reporter)

Comment 31

10 months ago
Posted image file-input-sizer.gif
Another example of the current crop not working well. When resizing we first begin clipping the end of the string (with no ellipsis), then eventually a middle crop begins, but the end still isn't visible. A little hard to explain by text so here's a gif.
(Reporter)

Updated

8 months ago
Attachment #8960044 - Attachment is obsolete: true
(Reporter)

Comment 33

8 months ago
Discussed with Mats about middle cropping the text in the label here:

> Implementing middle cropping for arbitrary content isn't easy.
> Bug 740910 has some ideas but none that are fully feasible, IIRC.

> Implementing something specifically for a single text node might
> be easier, especially if it's NAC (no selection or DOM access to
> worry about).
> Then you can just measure the width of the text, chop off characters
> in the middle if it's too wide, and repeat.  We have code to measure
> the width of a string without having to Reflow a frame for it.
> Given that we already have nsFileControlFrame for this, it seems it
> should be possible to override Reflow there and implement something
> like that.
(Reporter)

Comment 34

8 months ago
I had an alternate idea which is to implement more of the base <label> behavior in C++ instead of XBL, such that we wouldn't require running JS in the content process for this case. Since we don't need accessKey formatting or handling here maybe we could remove the nsIDOMXULDescriptionElement and nsIDOMXULLabelElement interfaces and instead move the accessible behavior directly into `description`/`label` tag name / attr lookups. Then we could support having a bare <label>, and then only require XBL/Custom Element for more complex cases (label-control and text-link). I think we may need to subclass XULElement in that case so the properties would get attached to the nodes.
Depends on: 1487313
Depends on: 1488116

Comment 35

8 months ago
(In reply to Brian Grinstead [:bgrins] from comment #34)
> I had an alternate idea which is to implement more of the base <label>
> behavior in C++ instead of XBL, such that we wouldn’t require running JS in
> the content process for this case. Since we don’t need accessKey formatting
> or handling here maybe we could remove the nsIDOMXULDescriptionElement and
> nsIDOMXULLabelElement interfaces and instead move the accessible behavior
> directly into `description`/`label` tag name / attr lookups. Then we could
> support having a bare <label>, and then only require XBL/Custom Element for
> more complex cases (label-control and text-link). I think we may need to
> subclass XULElement in that case so the properties would get attached to the
> nodes.

It certainly wouldn’t be without precedent.

(see bug 1437638 and bug 1446961)
Depends on: 1488620
when this bug is fixed, make sure to update minimal-xul.css to move :root text-rendering styles to xul.css, see bug 1446831 comment #11.
(Reporter)

Updated

7 months ago
Depends on: 1495153
(Reporter)

Comment 37

7 months ago
This was fixed by Bug 1495153
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Flags: needinfo?(shorlander)
Resolution: --- → FIXED
Attachment #9005057 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.