Closed Bug 309246 Opened 19 years ago Closed 18 years ago

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

Categories

(Core Graveyard :: XForms, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: surkov, Assigned: allan)

Details

(Keywords: fixed1.8, fixed1.8.0.2, fixed1.8.1)

Attachments

(2 files, 6 obsolete files)

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
Summary: "XForms Error" should can open JavaScript Console → "XForms Error" dialog should can open JavaScript Console
I haven't tested this, but I had the same thought yesterday. I think it would be
ok. What do you others say?
Sounds good.
fine with me
Attached file style (obsolete) —
Attached file dialog (obsolete) —
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).
Looks ok, can you supply a patch file?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Attached patch patch (obsolete) — Splinter Review
Somebody wants to review the patch :)?
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?
Attachment #197142 - Flags: review?(allan)
(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?
Description should be probably replaced by label.text-link element (see
http://xulplanet.com/references/elemref/ref_label.html#class_text-link).
Attached patch patch (obsolete) — Splinter Review
Attachment #197142 - Attachment is obsolete: true
Attachment #197505 - Flags: review?(allan)
Attachment #197142 - Flags: review?(allan)
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #196976 - Attachment is obsolete: true
Attachment #196982 - Attachment is obsolete: true
Attachment #197505 - Attachment is obsolete: true
Attachment #197665 - Flags: review?(allan)
Attachment #197505 - Flags: review?(allan)
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.
Attachment #197665 - Flags: review?(allan) → review+
(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.
Attached patch patch without text-link styles (obsolete) — Splinter Review
Attachment #197810 - Flags: review?(allan)
Comment on attachment 197810 [details] [diff] [review]
patch without text-link styles

r=me without the bogus changes to the .css file
Attachment #197810 - Flags: review?(smaug)
Attachment #197810 - Flags: review?(allan)
Attachment #197810 - Flags: review+
Comment on attachment 197810 [details] [diff] [review]
patch without text-link styles

r=me without extra newline additions.
Attachment #197810 - Flags: review?(smaug) → review+
Attached patch patchSplinter Review
Attachment #197665 - Attachment is obsolete: true
Attachment #197810 - Attachment is obsolete: true
Attachment #197971 - Flags: review?(allan)
Checked in to trunk
Assignee: aaronr → allan
Whiteboard: xf-to-branch
Status: NEW → ASSIGNED
Attachment #197971 - Flags: review?(allan)
checked into branch 20051004
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Whiteboard: xf-to-branch
Summary: "XForms Error" dialog should can open JavaScript Console → "XForms Error" dialog should have link to open JavaScript Console
Keywords: fixed1.8
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.
Oh, and as the dialog opens *before* logging the error to the console, opening the console is not much help in any case.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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?
(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?
(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);
}
Patch with Neil comments.

Aaron, please review the patch and say described problems from bug 316609 still exist?
Attachment #203227 - Flags: review?(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.
Attachment #203227 - Flags: review?(aaronr) → review+
(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.
Attachment #203227 - Flags: review+
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
Attachment #203227 - Attachment description: patch → patch that fix open()
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();"
can we close this one now since we are using bug 316609 for the a11y stuff?
(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.
Status: REOPENED → RESOLVED
Closed: 19 years ago18 years ago
Resolution: --- → FIXED
Should Neil's comments be fixed in bug 316609 too?
(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.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: