463080 - unifinder / views focus cleanup

Status: NEW

(Calendar :: Calendar Frontend, defect)

Description

[Przemyslaw Bialik]

STR:

1. Create 2 events in (at least) different weeks
2. Switch to day or week view
3. Move between events in unifinder by up / down key

Actual Results:
Event (in unifinder) receives double focus.

Expected Results:
Event gets single focus

Comment 1

[Przemyslaw Bialik]

Attachment: Fix v1

Focus introduced in attachment 343842 [details] [diff] [review] should be enough.

Comment 2

[Martin Schröder [:mschroeder]]

Comment on attachment 346293 [details] [diff] [review]
Fix v1

The focus you remove here is not related to an event in the unifinder, but to the whole view. I don't think this patch makes sense, but feel free to persuade me of the opposite.

Comment 3

[Przemyslaw Bialik]

You did manage to confirm the bug, right ?
I didn't saw any side effects after removal of this focus so if it can't be removed can you please tell me what's the real purpose of it ?

Comment 4

[Martin Schröder [:mschroeder]]

(In reply to comment #3)
> You did manage to confirm the bug, right ?
I see a refresh of the unifinder tree which obviously shouldn't be needed. But this is also true in other cases.

> I didn't saw any side effects after removal of this focus so if it can't be
> removed can you please tell me what's the real purpose of it ?
After some investigation I still can't tell you. But you agree that it doesn't fix the double focus problem?

Comment 5

[Stefan Sitter [:ssitter]]

The code in question was added to fix Bug 346292. 
Please verify that you don't regress that bug by removing the code.

Comment 6

[Stefan Sitter [:ssitter]]

Either I don't understand the reported issue completely or it works for me using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b2pre) Gecko/20081120 Calendar/1.0pre. Only one event in the unifinder has the focus, everything looks normal.

Comment 7

[Przemyslaw Bialik]

Attachment: Test ics

Ics with events showing the problem (import into new calendar and do the STR)

Comment 8

[Przemyslaw Bialik]

> I see a refresh of the unifinder tree which obviously shouldn't be needed. But
> this is also true in other cases.
The problem here is that focus goes to the view and back to the unifinder.
What cases ?

> But you agree that it doesn't fix the double focus problem?
it does for me and my testcase, else I wouldn't attach the patch

> The code in question was added to fix Bug 346292. 
> Please verify that you don't regress that bug by removing the code.
thanks for the bug id
I've found that with my patch after changing view with alt+ focus remains in unifinder - I guess this is not wanted ? (might be different opinions on this one)
if yes then the proper fix would have to not focus the views when changing them with keyboard and being in unifinder hmm

Comment 9

[Przemyslaw Bialik]

> if yes then the proper fix would have to not focus the views when changing them
> with keyboard and being in unifinder hmm
sorry, should be:
if yes then the proper fix would have to focus the views when changing them with keyboard and being in unifinder /and/ NOT focus the views when moving through events in unifinder
(unless I missed sth)

Comment 10

[Przemyslaw Bialik]

Comment on attachment 346293 [details] [diff] [review]
Fix v1

Bad patch

Comment 11

[Przemyslaw Bialik]

var focusedElement = document.commandDispatcher.focusedElement;
    if (focusedElement.getAttribute("id") != "unifinder-search-results-tree")
      currentView().focus();

this code fixes this bug and partially Bug 346292, IF has to be enhanced for the case of unifinder results tree having focus and user changing views (with keyboard or by menu) though I'm not sure how to check if the view day-week-multiweek-month has been changed, any hints ?
Also Bug 348481 could be backed out as this bug seems to be the root cause.

Comment 12

[Przemyslaw Bialik]

adjusting summary as I'm working on more complete patch (also previous comment is closed)

Comment 13

[Przemyslaw Bialik]

Attachment: Proposed fix v2

Adds focus where it should be - fixes this bug and does not regress Bug 346292

If agreed I would like to attach enh ver of the patch with the following change
/calendar/base/content/widgets/minimonth.xml
-                            oncommand="this.kMinimonth.selectDate(new Date());"
+                            oncommand="goToDate(now());" 
With this go to today would refresh the calendar view

Comment 14

[Martin Schröder [:mschroeder]]

(In reply to comment #13)
> If agreed I would like to attach enh ver of the patch with the following change
> /calendar/base/content/widgets/minimonth.xml
> -                            oncommand="this.kMinimonth.selectDate(new
> Date());"
> +                            oncommand="goToDate(now());" 
> With this go to today would refresh the calendar view

Please add a patch for this issue to bug 466979.

Comment 15

[Berend Cornelius]

Comment on attachment 359096 [details] [diff] [review]
Proposed fix v2

patch looks good. I could not detect any bad side-effects mentioned before. r=berend

Comment 16

[Berend Cornelius]

patch pushed to 
http://hg.mozilla.org/comm-central/rev/a9eba8e822a1

-> fixed. Thank you for your contribution

Comment 17

[Stefan Sitter [:ssitter]]

Looks like this checkin regressed Bug 476212.

Comment 18

[Stefan Sitter [:ssitter]]

Regressed Bug 476223 too.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

This bug has caused a lot of regressions, we should consider backing out the patch.

Comment 20

[Przemyslaw Bialik]

I'm sorry for not finding these regressions, they are way higher than this bug was originally about so I think it would be a good idea to back out the patch and I will try to work out a better one.

Comment 21

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

No problem at all, it happens.

Backed out in changeset http://hg.mozilla.org/comm-central/rev/a62cac409cf7

Comment 22

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Przemyslaw, do you want to take another stab at this bug? I would appreciate :-)

Comment 23

[Przemyslaw Bialik]

Sorry Philipp, I'm currently little out of Mozilla world so if anybody wants feel free to take it

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

These bugs are likely targeted at Lightning 1.0b1, not Lightning 1.0. If this change was done in error, please adjust the target milestone to its correct value. To filter on this bugspam, you can use "lightning-10-target-move".