309246 - "XForms Error" dialog should have link to open JavaScript Console

Status: RESOLVED FIXED

(Core Graveyard :: XForms, defect)

Description

[alexander :surkov (:asurkov)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b4) Gecko/20050908 Firefox/1.4

You can do something like this:

bindingex.xul

<description class="descr2" 
  onclick="open('chrome://global/content/console.xul', 'global:console', 
'menubar=no,resizable=yes')">
  &xforms.bindingdialog.description2;
</description>

bindingex.css

.descr2 { 
  font-size: 0.8em;
  color: blue;
} 

.descr2:hover{
	cursor: pointer;
}

Reproducible: Always

Comment 1

[Allan Beaufour]

I haven't tested this, but I had the same thought yesterday. I think it would be
ok. What do you others say?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Sounds good.

Comment 3

[aaronr]

fine with me

Comment 4

[alexander :surkov (:asurkov)]

Attachment: style

Comment 5

[alexander :surkov (:asurkov)]

Attachment: dialog

Please test it and correct styles and content as right for you.

I'm using open() method instead of openDialog() because it seems to me "XForms
Error" dialog is opened modal (bug 256555).

Comment 6

[Allan Beaufour]

Looks ok, can you supply a patch file?

Comment 7

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 8

[alexander :surkov (:asurkov)]

Somebody wants to review the patch :)?

Comment 9

[Allan Beaufour]

Comment on attachment 197142 [details] [diff] [review]
patch

>diff -uNr content.orig/bindingex.css content/bindingex.css
>--- content.orig/bindingex.css	2005-09-23 13:34:00.000000000 +0900
>+++ content/bindingex.css	2005-09-23 13:47:30.000000000 +0900

The file has Windows line-endings, you need to convert it to Unix-style.

>@@ -4,4 +4,9 @@
> 
> .descr2 { 
>   font-size: 0.8em;
>+  color: blue;

I do not know whether there is a "link"-class we could use instead, to use the
generic styling of links?

>+}
>+
>+.descr2:hover{

Add space before {

>+	cursor: pointer;

Do not use tabs.

> }
>\ В конце файла нет новой строки

What's this?

Comment 10

[alexander :surkov (:asurkov)]

(In reply to comment #9)
> I do not know whether there is a "link"-class we could use instead, to use the
> generic styling of links?
> 

There is link styles at chrome://communicator/skin/formatting.css. I think
xforms shouldn't depend on communicator styles. Threafore thease styles can be
included at bindings.css directly. Is it satisfactory solution?

Comment 11

[alexander :surkov (:asurkov)]

Description should be probably replaced by label.text-link element (see
http://xulplanet.com/references/elemref/ref_label.html#class_text-link).

Comment 12

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 13

[Allan Beaufour]

Comment on attachment 197505 [details] [diff] [review]
patch

You still use Windows line-endings.

>diff -uNr mozilla.orig/extensions/xforms/resources/content/bindingex.css mozilla/extensions/xforms/resources/content/bindingex.css
>--- mozilla.orig/extensions/xforms/resources/content/bindingex.css	2005-09-27 12:35:58.000000000 +0900
>+++ mozilla/extensions/xforms/resources/content/bindingex.css	2005-09-27 12:35:13.000000000 +0900

You do not need the styles. They are included automatically, just set the
"text-link" class on the label. That is enough. That way, a user can also style
text-link as he wants.

>diff -uNr mozilla.orig/extensions/xforms/resources/content/bindingex.xul mozilla/extensions/xforms/resources/content/bindingex.xul
>--- mozilla.orig/extensions/xforms/resources/content/bindingex.xul	2005-09-27 12:06:09.000000000 +0900
>+++ mozilla/extensions/xforms/resources/content/bindingex.xul	2005-09-27 12:35:50.000000000 +0900
>@@ -53,9 +53,9 @@
>       <description class="header descr1">
>         &xforms.bindingdialog.description1;
>       </description>
>-      <description class="descr2">
>+      <label class="descr2 text-link" onclick="open('chrome://global/content/console.xul', 'global:console', 'menubar=no,resizable=yes')">
>         &xforms.bindingdialog.description2;
>-      </description>
>+      </label>
>     </vbox>
>   </hbox>
> </dialog>

Fine. Only problem is that the label gets focus by default... I wonder whether
you can instruct the dialog to give the button focus as default? With that
fixed, I'm fine with this patch.

Comment 14

[alexander :surkov (:asurkov)]

Attachment: patch

I hope now I use Unix line-endings :)

> You do not need the styles. They are included automatically, just set the
> "text-link" class on the label. That is enough. That way, a user can also
style
> text-link as he wants.

I don't agree. They will not be included automatically if I run application
with -chrome command line argument. In that case communicator styles aren't
attached. In any case user can style text-link.

Comment 15

[Allan Beaufour]

Comment on attachment 197665 [details] [diff] [review]
patch

Good focus fix.

The -chrome issue is a special case in my view. We should look like any other
"thing" in Firefox, thus follow the standard styles.

Remove the css, and you have a r=me.

Comment 16

[alexander :surkov (:asurkov)]

(In reply to comment #15)
> The -chrome issue is a special case in my view. We should look like any other
> "thing" in Firefox, thus follow the standard styles.
> 
> Remove the css, and you have a r=me.
> 

As I understand you wants to see xforms as thing for browser. I think it's
ignominious with respect to people who used xforms in application. Application
is situated in own window (not in browser window) and thus link will not be
looked as link. I would propose to include browser styles into xforms dialog but
sometimes I guess browser package can be absent.

Truth to tell I think link styles should be included in global styles and
threafore I should remove them. I posted a bug 310403.

Comment 17

[alexander :surkov (:asurkov)]

Attachment: patch without text-link styles

Comment 18

[Allan Beaufour]

Comment on attachment 197810 [details] [diff] [review]
patch without text-link styles

r=me without the bogus changes to the .css file

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 197810 [details] [diff] [review]
patch without text-link styles

r=me without extra newline additions.

Comment 20

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 21

[Allan Beaufour]

Checked in to trunk

Comment 22

[aaronr]

checked into branch 20051004

Comment 23

[[e-mail]]

Comment on attachment 197971 [details] [diff] [review]
patch

>+        onload="document.getElementById('XFormsBindingDialog').getButton('accept').focus();">
This could use document.documentElement.

>+      <label class="descr2 text-link" onclick="open('chrome://global/content/console.xul', 'global:console', 'menubar=no,resizable=yes')">
This is not the right way to open the JS console. Not only does it not solve the modality issue it actually opens a browser window and loads the JS console inside it. Note that the global:console is a red herring because it's not a windowtype.

Comment 24

[[e-mail]]

Oh, and as the dialog opens *before* logging the error to the console, opening the console is not much help in any case.

Comment 25

[alexander :surkov (:asurkov)]

(In reply to comment #23)
> (From update of attachment 197971 [details] [diff] [review] [edit])
> >+        onload="document.getElementById('XFormsBindingDialog').getButton('accept').focus();">
> This could use document.documentElement.
> 

Ok

> >+      <label class="descr2 text-link" onclick="open('chrome://global/content/console.xul', 'global:console', 'menubar=no,resizable=yes')">
> This is not the right way to open the JS console. Not only does it not solve
> the modality issue it actually opens a browser window and loads the JS console
> inside it. Note that the global:console is a red herring because it's not a
> windowtype.
> 

Why doesn't open() method solve the modality issue and why is open() method so bad for js console opening? What the way should I choose to open js console?

Comment 26

[alexander :surkov (:asurkov)]

(In reply to comment #24)
> Oh, and as the dialog opens *before* logging the error to the console, opening
> the console is not much help in any case.
> 

I guess some time before it worked. Does anybody know what was happen?

Comment 27

[[e-mail]]

(In reply to comment #25)
>What is the way should I choose to open js console?
function toJSConsole() {
  var js = Components.classes["@mozilla.org/appshell/window-mediator;1"]
                     .getService(Components.interfaces.nsIWindowMediator)
                     .getMostRecentWindow('global:console');
  if (js)
    js.focus();
  else
    Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
              .getService(Components.interfaces.nsIWindowWatcher)
              .openWindow(null, "chrome://global/content/console.xul",
                          "", "all,dialog=no", null);
}

Comment 28

[alexander :surkov (:asurkov)]

Attachment: patch that fix open()

Patch with Neil comments.

Aaron, please review the patch and say described problems from bug 316609 still exist?

Comment 29

[aaronr]

(In reply to comment #28)
> Created an attachment (id=203227) [edit]
> patch
> 
> Patch with Neil comments.
> 
> Aaron, please review the patch and say described problems from bug 316609 still
> exist?
> 

This patch fixes the problem where tabbing to the label and pressing 'enter' doesn't work on Firefox.  We still need some visual indicator that it is something that can be tabbed too or clicked on.  Like in Seamonkey, the color of the label is black and it doesn't get any kind of focus indicator when you tab to it.

Comment 30

[alexander :surkov (:asurkov)]

(In reply to comment #29)

> We still need some visual indicator that it is
> something that can be tabbed too or clicked on.  Like in Seamonkey, the color
> of the label is black and it doesn't get any kind of focus indicator when you
> tab to it.
> 

Do you see a visual problem in seamonkey only? If it is true then problem will go away when bug 310403 will be fixed.

Comment 31

[Allan Beaufour]

Comment on attachment 203227 [details] [diff] [review]
patch that fix open()

I've checked this in to trunk, the visual a11y stuff goes on bug 316609

Comment 32

[[e-mail]]

Comment on attachment 203227 [details] [diff] [review]
patch that fix open()

>+    function toJSConsole() {
...
>+      }
Nit: } doesn't line up with function

>+      <label class="descr2 text-link" onclick="toJSConsole()">
While I'm nitpicking, it's neater to write "toJSConsole();"

Comment 33

[aaronr]

can we close this one now since we are using bug 316609 for the a11y stuff?

Comment 34

[Allan Beaufour]

(In reply to comment #33)
> can we close this one now since we are using bug 316609 for the a11y stuff?

Well, the last fix was only checked in on trunk. I just checked it in on 1_8_0 and 1_8. Closing it.

Comment 35

[alexander :surkov (:asurkov)]

Should Neil's comments be fixed in bug 316609 too?

Comment 36

[Allan Beaufour]

(In reply to comment #35)
> Should Neil's comments be fixed in bug 316609 too?

It's nits, but if you remember to fix it in bug 316609 that would be nice.