Closed
Bug 1457027
Opened 7 years ago
Closed 7 years ago
Create a HandlerListItem class for richlistbox#handlersView
Categories
(Toolkit :: XUL Widgets, task, P1)
Toolkit
XUL Widgets
Tracking
()
RESOLVED
FIXED
mozilla62
Tracking | Status | |
---|---|---|
firefox62 | --- | fixed |
People
(Reporter: zbraniecki, Assigned: Paolo)
References
Details
Attachments
(8 files)
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
jaws
:
review+
|
Details |
THe richlistbox in main.xul is very complicated and cannot be easily refactored to work with Fluent.
This makes it a blocker for bug 1435915.
There's a plan to extend Fluent to support the required feature [0] but it is scheduled for post Fluent 1.0, so I'd like to try to fix it without dynamic references.
If we could move richlistbox away from XBL, or at least flatten the XBL it uses, I believe we could migrate a major remaining chunk of .properties in Preferences to Fluent.
[0] https://github.com/projectfluent/fluent/issues/80
Reporter | ||
Updated•7 years ago
|
Blocks: war-on-xbl, 1435915
Updated•7 years ago
|
Component: XBL → XUL Widgets
Product: Core → Toolkit
Comment 1•7 years ago
|
||
Just to confirm - you are referring to the 'handler' XBL binding, right? https://searchfox.org/mozilla-central/rev/78dbe34925f04975f16cb9a5d4938be714d41897/browser/components/preferences/handlers.xml#13.
Will removing the custom XBL binding on richlistitems inside of #handlersView be enough, or do you want #handlersView to not be a richlistbox at all anymore?
Flags: needinfo?(gandalf)
Reporter | ||
Comment 2•7 years ago
|
||
If I could set the typeDescription and actionDescription on two elements, like this:
<foo>
<label id="type-description"/>
<label id="action-description-label"/>
<menulist />
</foo>
than I think it'll be totally enough.
Flags: needinfo?(gandalf)
Updated•7 years ago
|
Priority: -- → P5
Comment 3•7 years ago
|
||
I'm guessing this'll be similar to other extended richlistitem bindings we've done in prefs, like Bug 1420990
Assignee | ||
Comment 4•7 years ago
|
||
It looks like the patch in bug 1435915 already provides the solution that I first thought of when seeing comment 2, however I might as well convert this binding now since we're planning to do it anyways.
Out of curiosity, what is the approach when you have to build a string like this, and then use it both for the "value" of one label and the "tooltip" of a different element? That was lost in the patch I saw, which I don't think is an issue in this case, but I'm wondering if the conversion out of XBL would make a difference in the complexity of doing this.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Flags: needinfo?(gandalf)
Priority: P5 → P1
Comment hidden (mozreview-request) |
Reporter | ||
Comment 6•7 years ago
|
||
> It looks like the patch in bug 1435915 already provides the solution that I first thought of when seeing comment 2, however I might as well convert this binding now since we're planning to do it anyways.
The solution there is pretty twisted and complicated. I'd prefer not to go that route and as I was trying I kept discovering one-offs and edge cases that didn't work.
> Out of curiosity, what is the approach when you have to build a string like this, and then use it both for the "value" of one label and the "tooltip" of a different element?
That's not an issue. In Fluent you'd do:
```
something-title =
.label = This is my label
something-button =
.tooltip = { something-title.label }
```
The issue is when you have a single Element with two translatable attributes that should be set independently:
```
<my-element typeDescription="Something" labelDescription="Something else"/>
```
and if both of those attributes are meant to be controlled from JS.
There's a proposal to add dynamic references to Fluent: https://github.com/projectfluent/fluent/issues/80 - but that won't happen probably until 63.
For that reason I'm trying to get to the point where I can set them as:
```
<my-element>
<labelDescription data-l10n-id="foo"/>
<typeDescription data-l10n-id="faa"/>
</my-element>
```
Can we get it in this case?
Flags: needinfo?(gandalf)
Assignee | ||
Comment 7•7 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> something-title =
> .label = This is my label
> something-button =
> .tooltip = { something-title.label }
I guess this means that a change like converting <button label=> to <input type=button value=> would require changing the localization files, and also the cross-references inside them, but I suppose this trade-off was taken into account when designing the localization format.
> <my-element typeDescription="Something" labelDescription="Something else"/>
> and if both of those attributes are meant to be controlled from JS.
Out of curiosity again, even if it's frowned upon, can you used the declarative API to process the strings without assigning them to an element, and then set the two attributes separately via JavaScript?
> <my-element>
> <labelDescription data-l10n-id="foo"/>
> <typeDescription data-l10n-id="faa"/>
> </my-element>
Since after the conversion off of XBL there will be ordinary elements instead of anonymous elements, as I understand it you should be able to set the localization attributes directly on the elements as usual.
Assignee | ||
Comment 8•7 years ago
|
||
https://dxr.mozilla.org/mozilla-central/rev/045452b30d72/browser/components/preferences/applications.js#1007-1009
Myk, the answer to this question of yours from bug 377784 may be a bit late, but... yes I think so. I'll look into this.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Reporter | ||
Comment 12•7 years ago
|
||
(In reply to :Paolo Amadini from comment #7)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #6)
> > something-title =
> > .label = This is my label
> > something-button =
> > .tooltip = { something-title.label }
>
> I guess this means that a change like converting <button label=> to <input
> type=button value=> would require changing the localization files, and also
> the cross-references inside them, but I suppose this trade-off was taken
> into account when designing the localization format.
Yes. When you localize DOM, you bind localization unit to an element. If you change the shape of the element, you're changing the social contract with the localizer and should provide a new localization unit.
https://firefox-source-docs.mozilla.org/intl/l10n/l10n/fluent_tutorial.html#social-contract
You can abstract it in your CE if you want by producing custom elements:
<my-widget>
<ok-button data-l10n-id="..."/>
<cancel-button data-l10n-id="..."/>
</my-widget>
and then as long as the shape of the public element doesn't change, the l10n stays.
or you can do this via attributes:
<my-widget data-l10n-id="..."/>
and then as long as the shape of the `<my-widget/>` element stays the same, the l10n stays.
The limitation we have right now is that we don't yet have a way to handle a single element with two attributes coming from dynamic assignments. It's something we will add in the future tho.
> > <my-element typeDescription="Something" labelDescription="Something else"/>
> > and if both of those attributes are meant to be controlled from JS.
>
> Out of curiosity again, even if it's frowned upon, can you used the
> declarative API to process the strings without assigning them to an element,
> and then set the two attributes separately via JavaScript?
Yes, it is a work-around that I can implement if we can't change the shape of the DOM here.
It's ok to do those workaround if the work required to avoid them outweights the cleanness of the solution. Once we switch to the runtime langpack switching tho, those places will remain in the old locale until restart (a minor issue I believe).
> Since after the conversion off of XBL there will be ordinary elements
> instead of anonymous elements, as I understand it you should be able to set
> the localization attributes directly on the elements as usual.
That's even better :)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 21•7 years ago
|
||
mozreview-review |
Comment on attachment 8971949 [details]
Bug 1457027 - Part 1 - Don't persist the last selected handler.
https://reviewboard.mozilla.org/r/240706/#review247608
Attachment #8971949 -
Flags: review?(jaws) → review+
Comment 22•7 years ago
|
||
mozreview-review |
Comment on attachment 8972876 [details]
Bug 1457027 - Part 2 - Define services using defineLazyServiceGetters.
https://reviewboard.mozilla.org/r/241430/#review247612
Attachment #8972876 -
Flags: review?(jaws) → review+
Comment 23•7 years ago
|
||
mozreview-review |
Comment on attachment 8973208 [details]
Bug 1457027 - Part 3 - Unify references to bundlePreferences.
https://reviewboard.mozilla.org/r/241692/#review247614
Attachment #8973208 -
Flags: review?(jaws) → review+
Comment 24•7 years ago
|
||
mozreview-review |
Comment on attachment 8972877 [details]
Bug 1457027 - Part 4 - Use class syntax for the HandlerInfoWrapper hierarchy.
https://reviewboard.mozilla.org/r/241432/#review247616
::: browser/components/preferences/in-content/main.js:2778
(Diff revision 3)
> - },
> + }
> -
> -
> - // Plugin Handling
>
> + // this.pluginName
Why is this.pluginName and this.handledOnlyByPlugin commented out?
If they don't need to be declared, the placement of these comments is awkward as there appears to be no reason to document them at this place within the file.
Attachment #8972877 -
Flags: review?(jaws) → review+
Comment 25•7 years ago
|
||
mozreview-review |
Comment on attachment 8973178 [details]
Bug 1457027 - Part 5 - Move _describeType to HandlerInfoWrapper.
https://reviewboard.mozilla.org/r/241670/#review247622
Attachment #8973178 -
Flags: review?(jaws) → review+
Comment 26•7 years ago
|
||
mozreview-review |
Comment on attachment 8973209 [details]
Bug 1457027 - Part 6 - Move _describePreferredAction to HandlerInfoWrapper.
https://reviewboard.mozilla.org/r/241694/#review247628
Attachment #8973209 -
Flags: review?(jaws) → review+
Comment 27•7 years ago
|
||
mozreview-review |
Comment on attachment 8973233 [details]
Bug 1457027 - Part 7 - Move action icon getters to HandlerInfoWrapper.
https://reviewboard.mozilla.org/r/241712/#review247630
Attachment #8973233 -
Flags: review?(jaws) → review+
Comment 28•7 years ago
|
||
mozreview-review |
Comment on attachment 8973234 [details]
Bug 1457027 - Part 8 - Add a HandlerListItem class.
https://reviewboard.mozilla.org/r/241714/#review247634
Attachment #8973234 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 29•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #24)
> Why is this.pluginName and this.handledOnlyByPlugin commented out?
>
> If they don't need to be declared, the placement of these comments is
> awkward as there appears to be no reason to document them at this place
> within the file.
I've been a bit too conservative with keeping things in the same place in the file. The proper location for these initializations and comments is in the class constructor, I moved them there.
Assignee | ||
Comment 30•7 years ago
|
||
I'll land this part of the refactoring separately. The next step is the conversion of the initialization code off of XBL, and I'll ask Brian to review it. We are considering using a parseDOM helper to create the item structure.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a888f0c57add25bad22e873bc1bb7af3b9f7afd8
Assignee | ||
Updated•7 years ago
|
Summary: Move richlistbox#handlersView off of XBL → Create a HandlerListItem class for richlistbox#handlersView
Comment 31•7 years ago
|
||
Pushed by paolo.mozmail@amadzone.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33e9a37ae5b4
Part 1 - Don't persist the last selected handler. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/8834a397305b
Part 2 - Define services using defineLazyServiceGetters. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/d3afbfe98a71
Part 3 - Unify references to bundlePreferences. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2dd752c009e
Part 4 - Use class syntax for the HandlerInfoWrapper hierarchy. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/93bda045ca0b
Part 5 - Move _describeType to HandlerInfoWrapper. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e98dcc160f8
Part 6 - Move _describePreferredAction to HandlerInfoWrapper. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/b29b502c0fce
Part 7 - Move action icon getters to HandlerInfoWrapper. r=jaws
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9d78f354b26
Part 8 - Add a HandlerListItem class. r=jaws
Comment 32•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/33e9a37ae5b4
https://hg.mozilla.org/mozilla-central/rev/8834a397305b
https://hg.mozilla.org/mozilla-central/rev/d3afbfe98a71
https://hg.mozilla.org/mozilla-central/rev/a2dd752c009e
https://hg.mozilla.org/mozilla-central/rev/93bda045ca0b
https://hg.mozilla.org/mozilla-central/rev/3e98dcc160f8
https://hg.mozilla.org/mozilla-central/rev/b29b502c0fce
https://hg.mozilla.org/mozilla-central/rev/e9d78f354b26
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox62:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Updated•7 years ago
|
Target Milestone: mozilla61 → mozilla62
Updated•5 years ago
|
Type: enhancement → task
You need to log in
before you can comment on or make changes to this bug.
Description
•