Overlay suite features onto toolkit view source window

RESOLVED FIXED in seamonkey2.1a2

Status

SeaMonkey
UI Design
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: neil@parkwaycc.co.uk, Assigned: Robert Kaiser)

Tracking

Trunk
seamonkey2.1a2

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

9 years ago
Component: XP Apps: GUI Features → UI Design
(Assignee)

Updated

7 years ago
Duplicate of this bug: 89168
(Assignee)

Comment 3

7 years ago
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.
Assignee: nobody → kairo
Status: NEW → ASSIGNED
Keywords: helpwanted
Target Milestone: --- → seamonkey2.1a2
(Assignee)

Updated

7 years ago
Depends on: 567587
(Assignee)

Comment 4

7 years ago
Created attachment 446921 [details] [diff] [review]
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!
Attachment #446921 - Flags: review?(neil)
(Assignee)

Comment 5

7 years ago
Note that this needs the bug 567587 patch or the file menu overlaying doesn't work correctly for partial source.
(Reporter)

Comment 6

7 years ago
(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.]
(Assignee)

Comment 7

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

Comment 8

7 years ago
Created attachment 446929 [details] [diff] [review]
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.
Attachment #446921 - Attachment is obsolete: true
Attachment #446929 - Flags: review?(neil)
Attachment #446921 - Flags: review?(neil)
(Reporter)

Comment 9

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

7 years ago
+/* 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?
(Assignee)

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

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

Comment 16

7 years ago
Created attachment 447079 [details] [diff] [review]
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.
Attachment #446929 - Attachment is obsolete: true
Attachment #447079 - Flags: review?(neil)
Attachment #446929 - Flags: review?(neil)
(Reporter)

Comment 17

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

Comment 18

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

Comment 19

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

Comment 20

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

Updated

7 years ago
Attachment #447079 - Flags: review?(neil) → review+
(Assignee)

Comment 21

7 years ago
Pushed as http://hg.mozilla.org/comm-central/rev/d9a57a60f080
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.