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

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Comment 1

[:aceman]

Attachment: 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.

Comment 2

[:aceman]

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.

Comment 3

[Richard Marti (:Paenglab)]

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.

Comment 4

[:aceman]

Attachment: patch v2

Thanks a lot, that worked!

Comment 5

[[e-mail]]

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.

Comment 6

[:aceman]

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.

Comment 7

[[e-mail]]

(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.)

Comment 8

[:aceman]

Attachment: 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.

Comment 9

[[e-mail]]

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?

Comment 10

[:aceman]

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

Comment 11

[[e-mail]]

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.

Comment 12

[:aceman]

Thanks.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5dcf471886e6

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[:aceman]

  // 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?

Comment 16

[[e-mail]]

Actually I double-checked and the month buttons don't change the datepicker's value, so it may be expected.

Comment 17

[:aceman]

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.

Comment 18

[:aceman]

Attachment: 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.

Comment 19

[[e-mail]]

(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? ;-)

Comment 20

[:aceman]

I am not sure he has permissions there. But you can do it if you have them.

Comment 21

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed and results available: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd1fecdf228f

Comment 22

[:aceman]

Thanks, that looks good now. Neil, can we check it in?

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/015e96e1fef0

Comment 24

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/015e96e1fef0