411754 - Overlay suite features onto toolkit view source window

Status: RESOLVED FIXED

(SeaMonkey :: UI Design, defect)

Description

[[e-mail]]

The toolkit Error Console and the proposed fix to bug 8589 both use the toolkit view source window. We should overlay it to restore our features so we can then remove our version that that they originally forked.

We should do the same thing for view partial source too, of course.

CCing smontagu to avoid spamming bug 8589 any further.

Comment 1

[Simon Montagu :smontagu]

Do you want to wait on this for bug 8589? I was going to attach a new patch on the lines of:
<pseudocode>
  if (prefs.view_source.editor.external)
     utils.openInExternalEditor(...);
  else
     openDialog(...); // (as in the original) 
</pseudocode>

Comment 3

[Robert Kaiser]

I have a patch for this locally, which is also switching us over to toolkit
view source completely.

The actual differences are minimal, just "New" and "Edit Page" in the File menu, basically. Toolkit uses find bar instead of a find window, for consistency with bug 97023 work, I'll also move the findbar to be at the top of the content instead of below it.

Comment 4

[Robert Kaiser]

Attachment: use toolkit view source and overlay suite functions on it

This patch should do it. I saw that we weren't showing any menubar any more on partial source when I overlayed our general style rules, so I needed to add the menubar too the features set when opening partial source from the context menu.

20 files changed, 258 insertions(+), 1648 deletions(-)

woo-hoo!

Comment 5

[Robert Kaiser]

Note that this needs the bug 567587 patch or the file menu overlaying doesn't work correctly for partial source.

Comment 6

[[e-mail]]

(In reply to comment #5)
> Note that this needs the bug 567587 patch or the file menu overlaying doesn't
> work correctly for partial source.
[Strictly speaking you don't have to nest the file popup inside the menu bar.]

Comment 7

[Robert Kaiser]

(In reply to comment #6)
> (In reply to comment #5)
> > Note that this needs the bug 567587 patch or the file menu overlaying doesn't
> > work correctly for partial source.
> [Strictly speaking you don't have to nest the file popup inside the menu bar.]

Right, but having the same ID there is better anyhow. Actually, I still wonder if there's a good way to not have that have double source at all - but that better goes into its own bug.

Comment 8

[Robert Kaiser]

Attachment: only disable menu items in partial source

In the first patch, I forgot to really only disable the menu items in partial source, should work correctly in this one.

Comment 9

[[e-mail]]

Comment on attachment 446929 [details] [diff] [review]
only disable menu items in partial source

>-  openDialog("chrome://navigator/content/viewSource.xul",
>+  openDialog("chrome://global/content/viewSource.xul",
>              "_blank",
>              "all,dialog=no",
>              url, charset, pageCookie);
...
>-    window.openDialog("chrome://navigator/content/viewPartialSource.xul",
>-                      "_blank", "scrollbars,resizable,chrome,dialog=no",
>+    window.openDialog("chrome://global/content/viewPartialSource.xul",
>+                      "_blank", "scrollbars,resizable,chrome,menubar,dialog=no",
>                       docUrl, docCharset, reference, aContext);
Might as well use "all,dialog=no" here too.

>+  if (/viewPartialSource\.xul$/.test(document.baseURI)) {
IIRC we normally test against location.

>+  editPage(window.content.location.href.substring(12), window, false);
Actually I removed the last two parameters from editPage in bug 332668 but I see I overlooked some of the callers. Also bug 165162 tried to make editPage strip off view-source: but the code was incorrect until I fixed it as part of bug 192299, however for neither bug did anyone think to fix the callers...

+<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
What do we use from this file?

>+<!ENTITY % navDTD SYSTEM "chrome://navigator/locale/navigator.dtd">
Hmm, a communicator overlay shouldn't really use a navigator dtd...

>+  <script type="application/javascript" src="chrome://editor/content/editorApplicationOverlay.js"/>
Or an editor script... but I think editorNavigatorOverlay is a bit bare ;-)

>+    <key id="key_close2" disabled="true"/>
Are you adding this id to toolkit?

>+  <menubar id="viewSource-main-menubar" class="chromeclass-menubar"
>+          grippytooltiptext="&menuBar.tooltip;" position="1">
Nit: I don't think position has any effect here.

>+        <!-- inserting two elements both after one other reverses their order -->
>+        <menuseparator insertafter="menu_savePage"/>
>+        <menuitem id="menu_editPage" insertafter="menu_savePage"
>+                  key="key_editPage" command="cmd_editPage"
>+                  label="&editPageCmd.label;" accesskey="&editPageCmd.accesskey;"/>
In theory you should be able to insert the menuseparator after the menu_editPage, but I'm not sure that the document realises that the menu_editPage exists yet when we want to insert the menuseparator, so it might be better to insertbefore the page setup menuitem instead.

Comment 10

[Philip Chee]

+/* make findbar appear above content */
+#appcontent {
+  -moz-box-direction: reverse;
+}

1. This is behavioural right? So should go in content so that third party themes don't have to replicate this.

2. Why not -moz-box-ordinal on the findbar?

Comment 11

[Robert Kaiser]

(In reply to comment #10)
> 1. This is behavioural right? So should go in content so that third party
> themes don't have to replicate this.

I thought about that, but slowing things down with creating and loading yet another style sheet seemed to be even worse. Also, given that in the case of Classic, actual theme style goes inherently along with the change and given that nothing at all breaks when this isn't done, I decided to live with having it in the theme. You might know I'm maintaining 3rd-party themes myself, so I know well what's going on in that area. ;-)

> 2. Why not -moz-box-ordinal on the findbar?

Because it was hard enough for me to find one way that would work. ordinal might even make more sense but I didn't find it when I needed it. I'll try and see if it works well, and leave the other part (first question) up to Neil.

Comment 12

[[e-mail]]

(In reply to comment #9)
>(From update of attachment 446929 [details] [diff] [review])
>>+  <menubar id="viewSource-main-menubar" class="chromeclass-menubar"
>>+          grippytooltiptext="&menuBar.tooltip;" position="1">
>Nit: I don't think position has any effect here.
Also attribute alignment nit.

Comment 13

[Robert Kaiser]

(In reply to comment #9)
> >+  editPage(window.content.location.href.substring(12), window, false);
> Actually I removed the last two parameters from editPage in bug 332668 but I
> see I overlooked some of the callers.

http://mxr.mozilla.org/comm-central/ident?i=editPage still has two more callers with the additional params, should I file a followup?

> +<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
> What do we use from this file?

menu_New and its subitems come in from there.

> >+<!ENTITY % navDTD SYSTEM "chrome://navigator/locale/navigator.dtd">
> Hmm, a communicator overlay shouldn't really use a navigator dtd...
> 
> >+  <script type="application/javascript" src="chrome://editor/content/editorApplicationOverlay.js"/>
> Or an editor script... but I think editorNavigatorOverlay is a bit bare ;-)

Hmm, I didn't feel like creating an own file just for duplicating the strings, esp. as we never build communicator without having navigator available - and editor, for that matter.
Our own view source was in navigator, but then it's used from mailnews as well, so I figured communicator might be a better place - but then, it causes those theoretical problems.

> >+    <key id="key_close2" disabled="true"/>
> Are you adding this id to toolkit?

I just realized this makes no sense the way I have it, I'd need to add our close item as well. I'll just kill that <key>.

> >+        <!-- inserting two elements both after one other reverses their order -->
> >+        <menuseparator insertafter="menu_savePage"/>
> >+        <menuitem id="menu_editPage" insertafter="menu_savePage"
> >+                  key="key_editPage" command="cmd_editPage"
> >+                  label="&editPageCmd.label;" accesskey="&editPageCmd.accesskey;"/>
> In theory you should be able to insert the menuseparator after the
> menu_editPage, but I'm not sure that the document realises that the
> menu_editPage exists yet when we want to insert the menuseparator, so it might
> be better to insertbefore the page setup menuitem instead.

I just wanted to make sure anything else (think add-ons) inserting "after save page" or "before page setup" and for some reason being applied before us (is that possible?) would mess that up and land between those two. Also, inserting both after the same element sounded like it made sense, but insertafter="menu_editPage" didn't seem to work on the separator.
Well, maybe it makes sense to use insertbefore after all, as what I'm trying to do is separating the print items from the rest, esp. as page setup also contains the word page, just as the ones before, but means something else with it.

Comment 14

[Robert Kaiser]

(In reply to comment #11)
> > 2. Why not -moz-box-ordinal on the findbar?
> 
> Because it was hard enough for me to find one way that would work. ordinal
> might even make more sense but I didn't find it when I needed it. I'll try and
> see if it works well, and leave the other part (first question) up to Neil.

Actually, I just looked into that, and it just makes things more complicated. The default -mox-box-ordinal-group is 1, so I'd need to apply a higher integer to everything _except_ #FindToolbar, i.e. to #content. Doesn't really sound better to me than just using -moz-box-direction: reverse there, but if Neil thinks ordinal group is better, I'll follow his opinion as a reviewer.

Comment 15

[Philip Chee]

>> In theory you should be able to insert the menuseparator after the
>> menu_editPage, but I'm not sure that the document realises that the
>> menu_editPage exists yet when we want to insert the menuseparator, so it might
>> be better to insertbefore the page setup menuitem instead.
> 
> I just wanted to make sure anything else (think add-ons) inserting "after save
> page" or "before page setup" and for some reason being applied before us (is
> that possible?) would mess that up and land between those two.

FYI that's the way extension authors do it when they want to make sure that the order of their menu items is in the correct order after their overlays are applied. So KaiRo is doing it correctly.

Comment 16

[Robert Kaiser]

Attachment: address review comments

This patch should address all the nits so far. I also added a few line breaks to respect the 80 character limit better.

Comment 17

[[e-mail]]

(In reply to comment #13)
> http://mxr.mozilla.org/comm-central/ident?i=editPage still has two more callers
> with the additional params, should I file a followup?
Well, it's only a bug, right? ;-)

> Hmm, I didn't feel like creating an own file just for duplicating the strings,
> esp. as we never build communicator without having navigator available - and
> editor, for that matter.
Well, this was just a theoretical note where while the menuitem shouldn't appear in a --disable-composer build, that would require that the editor overlays be organised correctly to allow that to work. (So for instance editorSomethingOverlay should overlay viewSource.xul and provide the edit menuitem and entities etc.)

> insertafter="menu_editPage" didn't seem to work on the separator.
No, and I'm not sure why :-( I was actually expecting you to use insertbefore on both the menu and separator, but this way works too.

Comment 18

[[e-mail]]

Comment on attachment 446929 [details] [diff] [review]
only disable menu items in partial source

FYI
>+      <menupopup id="filemenu-popup">
...
>+        <!-- inserting two elements both after one other reverses their order -->
>+        <menuseparator insertafter="menu_savePage"/>
>+        <menuitem id="menu_editPage" insertafter="menu_savePage"
>+                  key="key_editPage" command="cmd_editPage"
>+                  label="&editPageCmd.label;" accesskey="&editPageCmd.accesskey;"/>
>+      </menupopup>
The IDs of inserted items can't be used for other items inserted in the same batch, because they are not registered until the end of the batch. But you can fake it using multiple batches. For instance:
<overlay>
  <menupopup id="filemenu-popup">
    <menuitem id="menu_editPage" insertafter="menu_savePage"
              key="key_editPage" command="cmd_editPage"
              label="&editPageCmd.label;" accesskey="&editPageCmd.accesskey;"/>
  </menupopup>
  <menupopup id="filemenu-popup">
    <menuseparator insertafter="menu_editPage"/>
  </menupopup>
</overlay>

Comment 19

[Robert Kaiser]

Neil, thanks for the explanation!

When I updated the patch, it came to my mind that actually having "Edit Page" after "Save Page" and a separator before all the printing items also makes sense logically, and so I think the solution in the patch is actually better than or just as good as this multi-batch method. What do you think?

Comment 20

[[e-mail]]

(In reply to comment #18)
> The IDs of inserted items can't be used for other items inserted in the same
> batch, because they are not registered until the end of the batch.
Apparently bug 564863 will change this, although it wasn't obvious to me.

Comment 21

[Robert Kaiser]

Pushed as http://hg.mozilla.org/comm-central/rev/d9a57a60f080