compose editor dialogs are always displayed on the left side of the screen

ASSIGNED
Assigned to

Status

ASSIGNED
2 years ago
9 days ago

People

(Reporter: aceman, Assigned: aceman, NeedInfo)

Tracking

Trunk
All
Linux

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

2 years ago
The composer editor dialogs are supposed to persist their location on the screen (respective to their opener window), using the function SetWindowLocation in EdDialogCommon.js . But on Linux only the vertical position is restored on reopen of the particular dialog.
This seems to be caused by bug 66483.
(Assignee)

Comment 1

2 years ago
Created attachment 8762272 [details] [diff] [review]
patch

Try run at https://hg.mozilla.org/try-comm-central/rev/77cc1d5b0e33d99f0733aacc50c537a1f4d02792 but I do not think we exercise the dialogs too much in the tests. Even the one test we have fails even without this patch.
Attachment #8762272 - Flags: ui-review?(richard.marti)
Attachment #8762272 - Flags: review?(mkmelin+mozilla)
Attachment #8762272 - Flags: review?(iann_bugzilla)
Comment on attachment 8762272 [details] [diff] [review]
patch

ui-r+

I've seen the jumping very seldom. Only on Cinnamon desktop I saw sometimes on the second or third opening of a dialog it was positioned at the left again and after that no more. But after closing TB and testing again the position was always correct.

On Kubuntu I couldn't repeat this behavior. So maybe a Cinnamon bug.
Attachment #8762272 - Flags: ui-review?(richard.marti) → ui-review+
(Assignee)

Comment 3

2 years ago
Strange, for me the position was never correct, without the patch (on KDE, gtk2 (TB45) and gkt3 (trunk)).
So does the patch fix it for you in all cases?
Without your patch it was also never correct. The behavior in my comment was with the patch applied.
Well, I'm not sure they really are supposed to remember their position. Isn't this just a case of a lot of dialogs not using the "centerscreen" in the opening arguments, while they really should?
(Assignee)

Comment 6

2 years ago
All the dialogs call the SetWindowLocation to explicitly set the position (not relying on automatic screenX and screenY persistence) relative to the parent compose window. Only about 3 (like search/replace or spell check) which rely on automatic persistence. Window the explicit code the position could persist, but would not move with parent compose window.

So it appears to me they are supposed to remember that. How would you otherwise interpret that code? It even stores the screenX and screenY in a separate id="location" element, not the <window>/<dialog>.
Yes at some point someone has thought that's a good idea. I'm asking why storing those would make any sense, instead of just using centerscreen?
(Assignee)

Comment 8

2 years ago
The feature still works on Win/OS X so you are proposing removing it (instead of not fixing it).

If someone uses them often, I can imagine placing the dialogs somewhere outside of compose window (which is not fullscreen) may be convenient to not obscure composition.

Comment 9

2 years ago
If we're fixing it here, is there anywhere else that will need it fixing (e.g. calendar)? If so should the workaround code be in a different file, so it can be used across the codebase or is it too small to matter?
Flags: needinfo?(acelists)
(Assignee)

Comment 10

2 years ago
If calendar uses the editor (I'd hope not) then it should inherit the fix automatically (it is in editor files). If you mean some other files that would have the same class of problem then that is possible. But this is some special positioning relative the to caller window which hopefully is not mimicked in other places.
Flags: needinfo?(acelists)
(In reply to :aceman from comment #8)
> The feature still works on Win/OS X so you are proposing removing it
> (instead of not fixing it).

Yes I think so.

> If someone uses them often, I can imagine placing the dialogs somewhere
> outside of compose window (which is not fullscreen) may be convenient to not
> obscure composition.

I find it quite unlikely to be useful, especially as the properties dialog is modal anyway...
Calendar dialogs rely on persistence or don't place anything at all, so this does not apply to calendar.
Comment on attachment 8762272 [details] [diff] [review]
patch

Review of attachment 8762272 [details] [diff] [review]:
-----------------------------------------------------------------

So what do other people think?
Attachment #8762272 - Flags: review?(mkmelin+mozilla)
I'm against using always centerscreen because I like the dialogs more almost on top of the screen to have the message text not hidden. It would be okay when the dialogs are placed centersceen initially but the position would be stored where the user placed them afterwards.

Comment 15

2 years ago
Magnus, who are you asking? I'm not on Linux, so I can't give you my opinion.

In general I'd like the compose dialog windows (Insert > Image, Table, Link, ...) to persist their location. This is already the case without the patch on Windows. It would be good if it were like this on all platforms. I've tried the patch on Windows and it doesn't change the existing behaviour, as intended, I suppose. So as i said, I'm in favour of fixing other platforms.
Not quite convinced but ok so can we make them centerscreen by default then. 
And why all the code? Shouldn't this simply be persist="screenX screenY" on the <dialog>?
(Assignee)

Comment 17

2 years ago
That only persists the absolute position on the screen.
"All the code" here saves the position relative to the opener (thus compose window). Try closing the dialog, then moving the compose window.
Yes, we can talk about whether this special feature is needed and absolute positioning is not enough.

Comment 18

2 years ago
Oh, I hadn't noticed that. Yes, the position is restored relative to the compose window. Maybe that's a bonus. I haven't managed to trick it by moving by - say - having the dialogue on the left screen edge and then moving the compose window to the left, so the relative position of the dialogue would be off-screen.

Are there other programs that persist like this relative to the opening window? The compose windows itself does *not* persist the position relative to the main window.

Comment 19

2 years ago
Comment on attachment 8762272 [details] [diff] [review]
patch

The code seems fine, at the end of the day this is a UI policy decision, so I would ask Philip Chee for a ui-review to from the SM side. r=me for the code.
Attachment #8762272 - Flags: review?(iann_bugzilla) → review+
(Assignee)

Updated

2 years ago
Attachment #8762272 - Flags: ui-review?(philip.chee)

Comment 20

2 years ago
Comment on attachment 8762272 [details] [diff] [review]
patch

> +      window.document.documentElement.setAttribute("persist", persist + "screenX screenY");
Shouldn't there be a space before screenX ?
Otherwise ur-r=me
Attachment #8762272 - Flags: ui-review?(philip.chee) → ui-review+
(Assignee)

Comment 21

2 years ago
(In reply to Philip Chee from comment #20)
> > +      window.document.documentElement.setAttribute("persist", persist + "screenX screenY");
> Shouldn't there be a space before screenX ?

Maybe. I wonder how it could be working if we concat offsetYscreenX together...
I'll look at that.
(Assignee)

Comment 22

2 years ago
(In reply to :aceman from comment #21)
> (In reply to Philip Chee from comment #20)
> > > +      window.document.documentElement.setAttribute("persist", persist + "screenX screenY");
> > Shouldn't there be a space before screenX ?
> 
> Maybe. I wonder how it could be working if we concat offsetYscreenX
> together...
> I'll look at that.

OK, it worked because the previous value of persist was usually empty, so the concatenation worked. But yes, let's make it properly robust.
(Assignee)

Comment 23

2 years ago
Created attachment 8792197 [details] [diff] [review]
patch v2

Adding persist attributes needs an ID on the dialog (otherwise nothing can be persisted) so I added some IDs where missing.
Attachment #8762272 - Attachment is obsolete: true
Attachment #8792197 - Flags: review?(jorgk)

Comment 24

2 years ago
Comment on attachment 8792197 [details] [diff] [review]
patch v2

Review of attachment 8792197 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine, but I don't just want to rs it. Try Magnus?
Really sorry.

::: editor/ui/dialogs/content/EdDialogCommon.js
@@ +551,5 @@
> +                                            Number(gLocation.getAttribute("offsetY")),
> +                                            screen.availHeight - window.outerHeight));
> +    }
> +    // On Linux, screenX and outerWidth do not work in an onload handler due to bug 66483.
> +    if (AppConstants.platform == "linux") {

I don't seem to be a suitable reviewer for this, sorry ;-(
In Berlin I have a Linux box, if you want to wait.
Attachment #8792197 - Flags: review?(jorgk)
(Assignee)

Comment 25

2 years ago
Comment on attachment 8792197 [details] [diff] [review]
patch v2

No problem, thanks.
Attachment #8792197 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8792197 [details] [diff] [review]
patch v2

Review of attachment 8792197 [details] [diff] [review]:
-----------------------------------------------------------------

I consistently see the "jumping around" of the dialogs, Ubuntu 16.04. You can see that it opens the dialog somewhere else, then moves it to wherever...

+ almost all the dialogs open in the wrong place, not centerscreen... even the ones I doubt I ever tried on this profile.
For the jumping around, especially Insert | Characters and Symbols. 

So it seems to have all the bad sides but not many upsides. You can't really count on the users to move the dialog to where they want - if the dialog is not initially correct they will just assume that thing is unpolished. And now we'd persist that place, so it wouldn't even be easily fixable in code.

::: editor/ui/dialogs/content/EdButtonProps.xul
@@ +10,5 @@
>  
>  <!DOCTYPE dialog SYSTEM "chrome://editor/locale/EditorButtonProperties.dtd">
>  <dialog title="&windowTitle.label;"
>          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +        id="buttonDlg"

maybe more specific ids would be helpful, like "edButtonPropsDlg"

This goes for all the other ids too. And can we please move them to first attribute?

::: editor/ui/dialogs/content/EdDialogTemplate.xul
@@ +11,5 @@
>  <!DOCTYPE dialog SYSTEM "chrome://editor/locale/Ed?????????.dtd">
>  <!-- dialog containing a control requiring initial setup -->
>  <dialog title="&windowTitle.label;"
>      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +    id="dialogTemplateDlg"

double dialog ?
Attachment #8792197 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 27

2 years ago
(In reply to Magnus Melin from comment #26)
> I consistently see the "jumping around" of the dialogs, Ubuntu 16.04. You
> can see that it opens the dialog somewhere else, then moves it to wherever...

That is strange. But you need to wait for the second open of the same dialog to see it appear in the correct place.

> + almost all the dialogs open in the wrong place, not centerscreen... even
> the ones I doubt I ever tried on this profile.
> For the jumping around, especially Insert | Characters and Symbols. 

For me they open centerscreen (or centered on compose window).

> So it seems to have all the bad sides but not many upsides. You can't really
> count on the users to move the dialog to where they want - if the dialog is
> not initially correct they will just assume that thing is unpolished. And
> now we'd persist that place, so it wouldn't even be easily fixable in code.
> 
> ::: editor/ui/dialogs/content/EdButtonProps.xul
> @@ +10,5 @@
> >  
> >  <!DOCTYPE dialog SYSTEM "chrome://editor/locale/EditorButtonProperties.dtd">
> >  <dialog title="&windowTitle.label;"
> >          xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > +        id="buttonDlg"
> 
> maybe more specific ids would be helpful, like "edButtonPropsDlg"

I kept the convention of naming, that already existed on some dialogs. Yes, they are not the best. Should I change also those that existed?

> This goes for all the other ids too. And can we please move them to first
> attribute?

Yes, with more churn :)

> 
> ::: editor/ui/dialogs/content/EdDialogTemplate.xul
> @@ +11,5 @@
> >  <!DOCTYPE dialog SYSTEM "chrome://editor/locale/Ed?????????.dtd">
> >  <!-- dialog containing a control requiring initial setup -->
> >  <dialog title="&windowTitle.label;"
> >      xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> > +    id="dialogTemplateDlg"
> 
> double dialog ?

Maybe. It seems like this file is never executed standalone (is unfinished template) and is an example for new dialogs to be created.
(Assignee)

Comment 28

2 years ago
(In reply to :aceman from comment #27)
> (In reply to Magnus Melin from comment #26)
> > I consistently see the "jumping around" of the dialogs, Ubuntu 16.04. You
> > can see that it opens the dialog somewhere else, then moves it to wherever...
> 
> That is strange. But you need to wait for the second open of the same dialog
> to see it appear in the correct place.

Tried with a new patch with new Ids (thus no preserved values) and it still works fine for me, no jumping.

> > + almost all the dialogs open in the wrong place, not centerscreen... even
> > the ones I doubt I ever tried on this profile.
> > For the jumping around, especially Insert | Characters and Symbols. 
> 
> For me they open centerscreen (or centered on compose window).

Actually the initial position is horizontally centered and a bit below the top of the calling compose window. The vertical location may be the offsetY=50 pre-declared in the xul files.
(Assignee)

Comment 29

2 years ago
Created attachment 8795902 [details] [diff] [review]
patch v3

Jorg, please also try on Linux if you see the flaws Magnus mentioned. I can't reproduce any problem.
Attachment #8792197 - Attachment is obsolete: true
Attachment #8795902 - Flags: review?(mkmelin+mozilla)
Attachment #8795902 - Flags: feedback?(jorgk)

Comment 30

2 years ago
(In reply to :aceman from comment #29)
> Jorg, please also try on Linux if you see the flaws Magnus mentioned. I
> can't reproduce any problem.
Sigh, will have to boot/update my Linux box for that. Actually, I don't have a Linux box, it's a dual boot with Windows.

Comment 31

2 years ago
Created attachment 8796053 [details]
Screenshot showing the drop down menu opens in wrong spot.

OK, I compiled on Linux (Mint 17.1 64bit) but I'm not sure what I should test.

When I open a new composition, the dialogues open in the centre of the composition.
If you move them, they reopen in the same position next time, but this position is relative to the compose window. So if you move the compose window, the dialogue will open in the same relative position to it next time, if it fits. Oh, I saw that I already said so in comment #18.

However, I noticed something terrible: Sometimes the pulldown opens detached from the menu item, but the submenu ("Insert") opens where it's supposed to be. That makes the system pretty unusable. See attached screenshot. Maybe that's another bug?

If there's something else you'd like me to test, please give clear instructions.

Comment 32

2 years ago
Comment on attachment 8795902 [details] [diff] [review]
patch v3

(In reply to Magnus Melin from comment #26)
> I consistently see the "jumping around" of the dialogs, Ubuntu 16.04. You
> can see that it opens the dialog somewhere else, then moves it to wherever...
The new function works for me. I didn't see anything that could be described as "jumping". For me, the dialogue opens and stays there.

More severe is the "shredded" menu.

I should mention that I'm using Mint's Cinnamon desktop.
Attachment #8795902 - Flags: feedback?(jorgk) → feedback+

Comment 33

2 years ago
Comment on attachment 8795902 [details] [diff] [review]
patch v3

Review of attachment 8795902 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/dialogs/content/EdDialogCommon.js
@@ +550,5 @@
> +      window.screenY = Math.max(0, Math.min(window.opener.screenY +
> +                                            Number(gLocation.getAttribute("offsetY")),
> +                                            screen.availHeight - window.outerHeight));
> +    }
> +    // On Linux, screenX and outerWidth do not work in an onload handler due to bug 66483.

Can you move this comment into the block. Also make this the else part, so:

if (... != "linux") {
  setPosition();
} else {
  // On Linux ...

@@ +560,5 @@
> +      let persist = window.document.documentElement.getAttribute("persist") || "";
> +      let persistAttrs = persist + " screenX screenY";
> +      window.document.documentElement.setAttribute("persist", persistAttrs.trim());
> +    } else {
> +      setPosition();

Can you move this code path first. Makes it easier to read.
Let me explain what I meant with the "jumping": it's that when you open the dialog you see it opening in one place and (presumably) once our custom code is executed it is moved to the persisted position. It's fairly fast but slow enough that it shouldn't be any problem noticing it.
(Assignee)

Comment 35

2 years ago
(In reply to Magnus Melin from comment #34)
> Let me explain what I meant with the "jumping": it's that when you open the
> dialog you see it opening in one place and (presumably) once our custom code
> is executed it is moved to the persisted position. It's fairly fast but slow
> enough that it shouldn't be any problem noticing it.

This should only happen if you move the compose window to a new location and then open the same editor dialog again. If you do not do this, no jumping should occur, because the persist attributes (on the <dialog> element) should open the dialog on the previous position. Then our code runs a moment later, and moves it to exactly the same position again (using the "location" element's persisted values). So no jumping should occur.

Only if you move the parent (compose window) between reopening the same dialog, then the dialog opening position and the final positions should differ. So I do not understand what happens in your case. Have you tried with the latest patch? The added ids are required to avoid the jumping (for persistence to work).

Comment 36

2 years ago
I checked again, I can't see any perceivable "jumping" on Mint 17.1 with Cinnamon. However, that may depend on the distro's desktop/window manager, right? I see the windows come up with a brief fade-up effect and they land where they should relative to the moved compose window.
(Assignee)

Comment 37

2 years ago
So what can we do here?
Thought I commented, but I must have forgot to submit. 

I didn't get the "juming" the next time I tried, but many dialogs do open up in wrong place initially. Those need to all be fixed to open on the correct place (centerscreen) initially or all we get by this is unpolished looking ui that persists the wrongness.
(Assignee)

Comment 39

2 years ago
(In reply to Magnus Melin from comment #38)
> Thought I commented, but I must have forgot to submit. 
> 
> I didn't get the "juming" the next time I tried, but many dialogs do open up
> in wrong place initially. Those need to all be fixed to open on the correct
> place (centerscreen) initially or all we get by this is unpolished looking
> ui that persists the wrongness.

So what do you mean by wrong place? Can you give an example?
Also, the code does not intend to place it centerscreen. There is an intentional offsetY=50 (vertically from the parent window). offsetX is not defined, so that would mean it opens aligned with the left edge of the parent window. Strangely that is not what I see, I see it centered horizontally. I can see if I can make the centering explicit.

Also, you do NOT see those problems without the patch?
Flags: needinfo?(mkmelin+mozilla)
(Assignee)

Comment 40

2 years ago
Created attachment 8799187 [details] [diff] [review]
patch v3.1

You can try this one.
Attachment #8795902 - Attachment is obsolete: true
Attachment #8795902 - Flags: review?(mkmelin+mozilla)
Attachment #8799187 - Flags: review?(mkmelin+mozilla)

Updated

9 days ago
See Also: → bug 1487435
You need to log in before you can comment on or make changes to this bug.