AB birthday field date picker is not properly disabled on LDAP card

RESOLVED FIXED in Firefox 44

Status

()

Toolkit
XUL Widgets
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
mozilla44
Points:
---

Firefox Tracking Flags

(firefox44 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

2.43 KB, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Updated

3 years ago
Component: Address Book → XUL Widgets
Product: Thunderbird → Toolkit
(Assignee)

Comment 1

3 years ago
Created attachment 8575525 [details] [diff] [review]
patch for toolkit

So while the text field can't be written to, the dropdown with the grid calendar can be opened. It can't be clicked and a date chosen, but the small arrows still allow to move months. This patch forbids that. Can be tested in TB or SM by opening a contact card on LDAP server.
Attachment #8575525 - Flags: review?(neil)
(Assignee)

Comment 2

3 years ago
Paenglab, can you add a separate patch here making the textfield in <datepicker> look disabled, similarly to the Year field that is right to it (in the TB AB contact properties dialog)? When the "readonly" or the "disabled" attribute is set to "true". I think the css change needs to be done in toolkit too.
Flags: needinfo?(richard.marti)
Make them look disabled/readonly is not possible through css because -moz-appearance: textfield is used. I'd need to set the appearance to none to style them but this would make them look different to the other textboxes. I tried to give the datetimepicker-input-box readonly="true" and it looked like then like the Year field.

Aceman, could you try to inherit the disabled/readonly from datepicker? Then this should work automatically.
Flags: needinfo?(richard.marti)
(Assignee)

Comment 4

3 years ago
Created attachment 8575619 [details] [diff] [review]
patch v2

Thanks a lot, that worked!
Attachment #8575525 - Attachment is obsolete: true
Attachment #8575525 - Flags: review?(neil)
Attachment #8575619 - Flags: review?(neil)

Comment 5

3 years ago
Comment on attachment 8575619 [details] [diff] [review]
patch v2

>           <button class="datepicker-previous datepicker-button" type="repeat"
>-                  xbl:inherits="disabled"
>-                  oncommand="document.getBindingParent(this)._increaseOrDecreaseMonth(-1);"/>
>+                  xbl:inherits="disabled,readonly"
>+                  oncommand="if (this.getAttribute('readonly') != 'true') document.getBindingParent(this)._increaseOrDecreaseMonth(-1);"/>
Look at how _increaseOrDecrease checks for read-only.
Attachment #8575619 - Flags: review?(neil) → review-
(Assignee)

Comment 6

3 years ago
Yes, and I have intentionally not implemented it that way. The documentation says that a readonly element is not changeable by user, but can be manipulated via script. If I would put the !this.readonly check inside _increaseOrDecreaseMonth() calling them from code wouldn't do anything. That seems to me to be agains the documentation. But yes, I see other functions do not care about that and seem to treat disabled and readonly the same. Unless there is also some distinction between functions with underscores and those without. So if you want me to implement it consistently to the rest of the methods (even if it is wrong), I can do that.
Flags: needinfo?(neil)

Comment 7

3 years ago
(In reply to aceman from comment #6)
> Yes, and I have intentionally not implemented it that way. The documentation
> says that a readonly element is not changeable by user, but can be
> manipulated via script. If I would put the !this.readonly check inside
> _increaseOrDecreaseMonth() calling them from code wouldn't do anything.
XBL does not have private methods, so the convention we use is that methods whose names begin with an underscore are private and should not be called by external code. (Similarly fields whose names begin with m are private.)
Flags: needinfo?(neil)
(Assignee)

Comment 8

3 years ago
Created attachment 8576793 [details] [diff] [review]
patch v2.1

While I think there could be some public method calling these internal methods so they should be accessible when readonly, I'm OK with keeping it consistent with other functions.
Attachment #8575619 - Attachment is obsolete: true
Attachment #8576793 - Flags: review?(neil)

Comment 9

3 years ago
Comment on attachment 8576793 [details] [diff] [review]
patch v2.1

>     <content align="center">
>       <xul:hbox class="datetimepicker-input-box" align="center"
>-                xbl:inherits="context">
>+                xbl:inherits="context,disabled,readonly">
Sorry, would you mind explaining what this change achieves?
(Assignee)

Comment 10

3 years ago
Should make the inputbox appear disabled, the attributes just need to be applied on the containing hbox. See comment 3.
(Assignee)

Updated

3 years ago
Flags: needinfo?(neil)
Comment on attachment 8576793 [details] [diff] [review]
patch v2.1

Sorry that it took me so long to get around to this, it could have been a really quick review too.
Flags: needinfo?(neil)
Attachment #8576793 - Flags: review?(neil) → review+
(Assignee)

Comment 12

3 years ago
Thanks.
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All

Comment 13

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/5dcf471886e6
Keywords: checkin-needed
Backed out for test_datepicker.xul failures. Please verify that this is green on Try before requesting checkin again.
https://treeherder.mozilla.org/logviewer.html#?job_id=3213875&repo=fx-team

https://hg.mozilla.org/integration/fx-team/rev/ee0dd817cd8a
(Assignee)

Comment 15

3 years ago
  // month can still be changed even when readonly
  synthesizeKeyExpectEvent("VK_PAGE_DOWN", { }, ktarget, "monthchange",
                           testid + "key page up read only");
  synthesizeKeyExpectEvent("VK_PAGE_UP", { }, ktarget, "monthchange",
                           testid + "key page down read only");

Well, the test specifically tests for the behaviour I am trying to disallow in this bug. Can we find out if this was intentionally designed that the month buttons should be usable?
Flags: needinfo?(neil)
Actually I double-checked and the month buttons don't change the datepicker's value, so it may be expected.
Flags: needinfo?(neil)
(Assignee)

Comment 17

3 years ago
Yes, they do not change the value. But a readonly or disabled picker still appears as enabled (is not grey). I'll try to rework the patch to make the appearance better and still keep the buttons active.
(Assignee)

Comment 18

3 years ago
Created attachment 8610829 [details] [diff] [review]
patch v2.2

This works for me. disabled=true makes the field grey (disabled) and also the calendar can't be opened. But readonly=true makes the field grey, but leaves the calendar and the buttons in it working, but unable to set the value. The calendar view is still white, but I could live with that :)

Aryx, can you push this m-c patch to TB-try? Supposedly m-c tests also run there.
Attachment #8576793 - Attachment is obsolete: true
Flags: needinfo?(archaeopteryx)
Attachment #8610829 - Flags: review?(neil)

Updated

3 years ago
Attachment #8610829 - Flags: review?(neil) → review+
(In reply to aceman from comment #18)
> Aryx, can you push this m-c patch to TB-try? Supposedly m-c tests also run
> there.

Why not just ask him to push it to m-c try in the first place? ;-)
(Assignee)

Comment 20

3 years ago
I am not sure he has permissions there. But you can do it if you have them.
Pushed and results available: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd1fecdf228f
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 22

3 years ago
Thanks, that looks good now. Neil, can we check it in?
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 23

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/015e96e1fef0
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/015e96e1fef0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.