Closed
Bug 1446830
Opened 7 years ago
Closed 6 years ago
Don't use a XUL label for displaying the selected filename in the filepicker UI
Categories
(Core :: XUL, task, P5)
Core
XUL
Tracking
()
RESOLVED
FIXED
People
(Reporter: bgrins, Unassigned)
References
(Blocks 1 open bug)
Details
(Whiteboard: [overhead:noted])
Attachments
(4 files, 3 obsolete files)
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 | ||
Comment 1•7 years 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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 4•7 years ago
|
||
mozreview-review |
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•7 years 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•7 years 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.
Reporter | ||
Comment 7•7 years 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•7 years 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•7 years 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•7 years ago
|
Blocks: war-on-xbl
Comment 10•7 years ago
|
||
There isn't any other text displaying frame that crops like that.
Flags: needinfo?(enndeakin)
Reporter | ||
Comment 11•7 years 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•7 years 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•7 years ago
|
Reporter | ||
Comment 13•7 years 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.
Comment 14•7 years ago
|
||
(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•7 years 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•7 years 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•7 years ago
|
||
Sure, any chance you could make a try build please? Thanks!
Reporter | ||
Comment 22•7 years 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•7 years 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•7 years 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•7 years 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...
Updated•7 years ago
|
Comment 26•7 years ago
|
||
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•7 years ago
|
Priority: -- → P5
Comment 28•7 years ago
|
||
Moving to Core:XUL per https://bugzilla.mozilla.org/show_bug.cgi?id=1455336
Component: XP Toolkit/Widgets: XUL → XUL
Updated•6 years ago
|
Whiteboard: [overhead:noted]
Comment hidden (mozreview-request) |
Reporter | ||
Comment 30•6 years 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•6 years ago
|
||
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 | ||
Comment 32•6 years ago
|
||
Reporter | ||
Updated•6 years ago
|
Attachment #8960044 -
Attachment is obsolete: true
Reporter | ||
Comment 33•6 years 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•6 years 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.
Comment 35•6 years 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)
Comment 36•6 years ago
|
||
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 | ||
Comment 37•6 years ago
|
||
This was fixed by Bug 1495153
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(shorlander)
Resolution: --- → FIXED
Updated•6 years ago
|
Attachment #9005057 -
Attachment is obsolete: true
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•