unifinder / views focus cleanup

NEW
Unassigned

Status

Calendar
Calendar Views
--
trivial
9 years ago
6 years ago

People

(Reporter: Przemyslaw Bialik, Unassigned)

Tracking

Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

991 bytes, text/calendar
Details
(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
Created attachment 346293 [details] [diff] [review]
Fix v1

Focus introduced in attachment 343842 [details] [diff] [review] should be enough.
Attachment #346293 - Flags: review?(philipp)
Attachment #346293 - Flags: review?(philipp) → review?(mschroeder)
(Reporter)

Updated

9 years ago
OS: Windows 2000 → All
Hardware: PC → All
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.
(Reporter)

Comment 3

9 years ago
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 ?
(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

9 years ago
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

9 years ago
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.
(Reporter)

Comment 7

9 years ago
Created attachment 349605 [details]
Test ics

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

Comment 8

9 years ago
> 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
(Reporter)

Comment 9

9 years ago
> 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)
(Reporter)

Comment 10

9 years ago
Comment on attachment 346293 [details] [diff] [review]
Fix v1

Bad patch
Attachment #346293 - Attachment is obsolete: true
Attachment #346293 - Flags: review?(mschroeder)
(Reporter)

Comment 11

9 years ago
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.
(Reporter)

Comment 12

9 years ago
adjusting summary as I'm working on more complete patch (also previous comment is closed)
Summary: unnecessary second focus in some cases when switching between events in unifinder → unifinder / views focus cleanup
(Reporter)

Comment 13

9 years ago
Created attachment 359096 [details] [diff] [review]
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
Attachment #359096 - Flags: review?(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.
Attachment #359096 - Flags: review?(mschroeder) → review?(Berend.Cornelius)

Updated

9 years ago
Attachment #359096 - Flags: review?(Berend.Cornelius) → review+

Comment 15

9 years ago
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

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

-> fixed. Thank you for your contribution
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 17

9 years ago
Looks like this checkin regressed Bug 476212.
Depends on: 476212

Comment 18

9 years ago
Regressed Bug 476223 too.
Depends on: 476223

Updated

9 years ago
Depends on: 476235
This bug has caused a lot of regressions, we should consider backing out the patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: --- → 1.0
(Reporter)

Comment 20

9 years ago
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.
No problem at all, it happens.

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

Updated

9 years ago
Attachment #359096 - Attachment is obsolete: true
(Reporter)

Updated

9 years ago
Assignee: firefox → nobody
Status: REOPENED → NEW
Przemyslaw, do you want to take another stab at this bug? I would appreciate :-)
(Reporter)

Comment 23

7 years ago
Sorry Philipp, I'm currently little out of Mozilla world so if anybody wants feel free to take it
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".
Target Milestone: 1.0 → 1.0b1
Target Milestone: 1.0b1 → ---
You need to log in before you can comment on or make changes to this bug.