Closed Bug 1141703 Opened 5 years ago Closed 4 years ago

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

Categories

(Toolkit :: XUL Widgets, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla44
Tracking Status
firefox44 --- fixed

People

(Reporter: aceman, Assigned: aceman)

Details

Attachments

(1 file, 3 obsolete files)

No description provided.
Component: Address Book → XUL Widgets
Product: Thunderbird → Toolkit
Attached patch patch for toolkit (obsolete) — Splinter Review
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)
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)
Attached patch patch v2 (obsolete) — Splinter Review
Thanks a lot, that worked!
Attachment #8575525 - Attachment is obsolete: true
Attachment #8575525 - Flags: review?(neil)
Attachment #8575619 - Flags: review?(neil)
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-
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)
(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)
Attached patch patch v2.1 (obsolete) — Splinter Review
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 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?
Should make the inputbox appear disabled, the attributes just need to be applied on the containing hbox. See comment 3.
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+
Thanks.
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
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
  // 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)
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.
Attached patch patch v2.2Splinter Review
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)
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? ;-)
I am not sure he has permissions there. But you can do it if you have them.
Thanks, that looks good now. Neil, can we check it in?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/015e96e1fef0
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla44
You need to log in before you can comment on or make changes to this bug.