Make Error Console work better with screen readers

RESOLVED WONTFIX

Status

Toolkit Graveyard
Error Console
RESOLVED WONTFIX
12 years ago
10 months ago

People

(Reporter: Aaron Leventhal, Unassigned)

Tracking

(Depends on: 1 bug, {access, helpwanted})

Trunk
access, helpwanted
Dependency tree / graph

Details

(Whiteboard: [patchlove][has draft patch][needs new assignee])

Attachments

(1 attachment, 11 obsolete attachments)

(Reporter)

Description

12 years ago
A few things to fix:
1. Make the Evaluate button a regular XUL button, so it's in the tab order (make
it the default button as well)
2. Use <toolbarbutton class="tabbable"> so that they are discoverable to screen
reader users (but it's probably best to have initial focus start in the text
field anyway).
3. Make the list a <richlistbox>, so that each item can be read aloud by a
screen reader as the user arrows to it.
(Reporter)

Updated

12 years ago
Summary: Make download manager work better with screen readers → Make javascript console work better with screen readers

Comment 1

12 years ago
A few things I ran into when trying to convert the console to using a
<richlistbox> (see http://forums.mozillazine.org/viewtopic.php?t=318102):
* For sorting, you'll have to reverse the childNodes using DOM functions, since
with the CSS hack used so far, the results of the up/down and home/end buttons
are switched as well (and since the reversed results wouldn't be accessible to
child iterating screen readers, anyway).

* For filtering, you'll have to modify goUp/goDown/scrollOnePage so as to skip
over hidden elements (hidden = !item.boxObject.height).

* Make sure that all richlistbox errors/strict warnings are fixed, otherwise
you'll unnecessarily clutter the console while navigating (there's a strict
warning in getIndexOf that'll probably make it into Firefox 1.5).

* If you don't use templates, make sure that you use the children property (used
in getItemAtIndex, getRowCount, getIndexOf, etc.) as rarely as possible -
iterating the nodes with firstChild/nextSibling or childNodes is much faster.

* Optionally replace the items' toString method with the label property.

Updated

11 years ago
Summary: Make javascript console work better with screen readers → Make Error Console work better with screen readers

Updated

10 years ago
Duplicate of this bug: 391341

Updated

10 years ago
Duplicate of this bug: 404349
I'm currently working on the richlistbox implementation and other console reworks. I'll try to address all the accessibility issues as well. (do screen readers read visibility:hidden items or are they smart enough to skip them?).

In any event, to completely fix this would probably require using a template, which I'll hopefully get to some time after the initial rework patch comes through.
Assignee: nobody → highmind63
Status: NEW → ASSIGNED
Hardware: x86 → All
Whiteboard: [patch in bug 489736]
Version: unspecified → Trunk
(In reply to comment #4)
> readers read visibility:hidden items or are they smart enough to skip them?).
s/visibility:hidden/visibility:collapse/

Comment 6

8 years ago
(In reply to comment #5)
> (In reply to comment #4)
> > readers read visibility:hidden items or are they smart enough to skip them?).
> s/visibility:hidden/visibility:collapse/

I'm definitely sure about visibility: hidden; that these elements are being skipped. Not sure about visibility: collapse. But you can see that in DOM Inspector if you turn on Acessibility and look at the Accessible Tree of the Error Console Chrome window.

Updated

8 years ago
Depends on: 490886
Whiteboard: [patch in bug 489736]
Created attachment 375295 [details] [diff] [review]
part 1: move to richlistbox

This is really the bulk of the patch, sorry but I don't really see another way of going about this, I hope this isn't too much in one patch!
Attachment #375295 - Flags: review?(zeniko)
Depends on bug 490499 for the sort ordering.
Depends on: 490499

Comment 9

8 years ago
Comment on attachment 375295 [details] [diff] [review]
part 1: move to richlistbox

Thanks for investing into the Error Console. A first review pass:

>+/* ***** BEGIN LICENSE BLOCK *****

Adding license boilerplate adds size without offering the user more functionality. The header should thus at least be removed by the precompiler, if it really has to be there.

>+richlistitem {
>+  visibility: collapse;

I'd prefer displaying everything by default and then collapse depending on the applied filter.

>+#console[mode="error"] > richlistitem[type="error"] {
>+  visibility: visible;
> }
>+#console[mode="warning"] > richlistitem[type="warning"] {
>+  visibility: visible;
> }

You can take these together (same logic, same rule). With the above set, it should however rather be:

>+#console[mode="error"] > richlistitem:not([type="error]) ...

>+richlistitem:not([lineNumber]) > box > .line-col-hbox,
>+richlistitem:not([errorSrcStart]) > box > vbox > .error-source,
>+richlistitem:not([colNumber]) > box > hbox > vbox > .console-col-number,
>+richlistitem:not([colNumber]) > box > hbox > vbox > .pre-col-label {
>+  visibility: collapse;

Any reason for not using "display: none" for these?

>+  QueryInterface: function FullZoom_QueryInterface(aIID) {

FullZoom? ;-)

>+    if (aIID.equals(Ci.nsIContentPrefObserver) ||

Content prefs? AFAICT you don't need this QueryInterface at all.

>+        aIID.equals(Ci.nsISupportsWeakReference) ||
>+        aIID.equals(Ci.nsISupports))
>+      return this;

General style nit: Please add braces when either the condition or the single statement span more than one line.

>+  chromePref: "showInConsole",

Please just inline this value. AFAICT you use it exactly once and I don't see any point in making this customizable.

>+  get showChrome() {

More readable: showChromeErrors?

>+  limit: 250,

In this context, "limit" sounds too generic (it's been before as well). Please rename it at this occasion.

>+  bundle: null,

Why bundle but not console? And why not a smart getter for these two as well (just as for the services above)?

>+  clearConsole: function console__clearConsole(aNotify) {

Please clarify "aNotify".

>+    while(this.console.children.length != 0)

Nit: No space after while.

>+    var messages = out.value;
>+    // In case getMessageArray returns 0-length array as null
>+    if (!messages)
>+      messages = [];

More concise: var messages = out.value || [];

>+  checkMessage: function console__checkMessage(aMessage) {

This function does a lot more than "check". processMessage?

>+        return null;

Nit: "function does not always return a value". Just "return;" ought to do (or even better: reverse the condition and drop the return).

>+        item.setAttribute("errorSrcStart", src[0].replace(/\s/g, " "));
>+        item.setAttribute("errorSrcEnd", src[1].replace(/\s/g, " "));

Two different variables instead of src[0] and src[1] would be more readable.

>+  showTab: function console__showTab(aName) {

s/showTab/setMode/ ?

>+  getItemText: function console__getItemText(aItem) {
>+    var char = "";

Nit: "char" is going to be a string value.

>+    if (type == "message")
>+      return char;

I'd prefer the inverted condition, so that it's clear for what types we continue ("error" and "warning"?).

>+    char += "\r\n";

Shouldn't that be just "\n" or at worst OS dependent?

>+    else if (aItem.lineNumber)
>+        char += this.bundle.getFormattedString("errLine", [aItem.lineNumber]);

Nit: Indentation.

>+    while(items[i] && mode != "all" && items[i].getAttribute("type") != mode)
>+      i++;
>+    if (items[i])
>+      char = this.getItemText(items[i++]);

You can drop these if you make "char" (renamed!) an Array and just push all selected items of the correct type(s). In the end, you can then |char.join(separator)|.

>+var consoleController = {

For consistency, this should be gConsoleController, shouldn't it?

>+  supportsCommand: function(aCommand) {
>+    return aCommand == "cmd_copy" ||
>+           aCommand == "cmd_selectAll";

A switch block would make this more readable (see other supportsCommand implementations).

>+      case "cmd_copy":
>+        return gConsole.console.selectedIndex != -1;
>+      break;

Nit: No need for break after return (unreachable code). And if there were a need, the break should be indented.

>+  doCommand: function(aCommand) {
>+   if (aCommand == "cmd_selectAll")

Please also use switch here.

>+  if (aMode != "all")
>+    aMode += "s";

This looks too fragile. What's wrong with "console-mode-error", etc.?

>+  <keyset id="editMenuKeys"/>

Unused?

>-      <menuseparator/>

Please keep the separator in the popup menu.

>+  </richlistbox>

Since this element doesn't have children, you could just <richlistbox .../>

>+  <binding id="console-message-error"
>+           extends="chrome://global/content/bindings/richlistbox.xml#richlistitem">
>+    <content>
>+      <xul:box flex="1" class="console-message-box">

Can't you do without this box (just applying styles to the richlistitem itself instead)?

>+        <xul:hbox class="image-box">
>+          <xul:vbox class="image-box" flex="1">
>+            <xul:image class="error-image" xbl:inherits="type"/>
>+          </xul:vbox>
>+        </xul:hbox>

One of the two "image-box"s ought to be enough.

>+          <xul:vbox>
>+            <xul:label class="console-message"
>+                       xbl:inherits="type,xbl:text=msg"/>
>+          </xul:vbox>

Any reason for the vbox around the label?

>+          <xul:vbox class="error-source">
>+            <xul:hbox>

Again: The hbox should be sufficient.

>+              <xul:vbox flex="1">
>+                <xul:label class="console-source" anonid="error-source-start"
>+                           crop="start" xbl:inherits="value=errorSrcStart"/>
>+                <xul:hbox>
>+                  <xul:label class="console-dots" crop="start" flex="1"/>
>+                  <xul:box class="error-caret"/>
>+                </xul:hbox>
>+              </xul:vbox>
>+              <xul:vbox flex="1">
>+                <xul:label class="console-source" anonid="error-source-end"
>+                           crop="end" flex="1" xbl:inherits="value=errorSrcEnd"/>
>+              </xul:vbox>

The original console achieved this through pure CSS. Is this no longer possible? Note: Code should continue to be set in a monospaced font.

>+      <field name="errorMsg">
>+        this.getAttribute("msg");
>       </field>

Shouldn't this be a property, so that the value is dynamically updated when the attribute changes? The same also applies to href, lineNumber, preLine, preCol, colNumber and errorSrc. Then again, are all of these needed at all?

>+          var url = this.href;
>+          var line = this.lineNumber;

Just pass the values in directly, no need for the intermediary variables.

>+#console {
>+  margin: 2px 0 0 0;

Magic numbers need a comment explaining them.

>+richlistitem {
>+  min-height: 45px;

Why 45px?

>+  margin-right: 0px !important;
>+  margin-left: 0px !important;

Why the need for !important ?

>+.console-link[selected="true"] {
>+  color: #000000;

I guess we need a system provided color instead here (-moz-FieldText ?), otherwise this could well be invisible on a high-contrast theme. What's the background color?

>+..console-message[type="message"] {

Nit: double dot.
Attachment #375295 - Flags: review?(zeniko) → review-
(In reply to comment #9)
> >+richlistitem:not([lineNumber]) > box > .line-col-hbox,
> >+richlistitem:not([errorSrcStart]) > box > vbox > .error-source,
> >+richlistitem:not([colNumber]) > box > hbox > vbox > .console-col-number,
> >+richlistitem:not([colNumber]) > box > hbox > vbox > .pre-col-label {
> >+  visibility: collapse;
> 
> Any reason for not using "display: none" for these?

In my experience, doing display:none on anon content had some drawbacks, I'll see if it makes any difference here (probably not).

> >+  chromePref: "showInConsole",
> 
> Please just inline this value. AFAICT you use it exactly once and I don't see
> any point in making this customizable.

I really use it a lot more in the full patch, didn't want to make too many conflicting differences between the split up patches if it really is only a nit. What do you mean by "making this customizable"? You don't want the checkboxes? If so, I can remove this as the usage really kicks in for the checkboxes.

> >+    char += "\r\n";
> 
> Shouldn't that be just "\n" or at worst OS dependent?

I'm still trying to clarify what exactly this will do on linux or mac boxes, is this catastrophic? does it make a double line? Because on Windows '\n' results in no lines at all :(

> >+  if (aMode != "all")
> >+    aMode += "s";
> 
> This looks too fragile. What's wrong with "console-mode-error", etc.?

...and pathetic! I will change this promptly...

> >+        <xul:hbox class="image-box">
> >+          <xul:vbox class="image-box" flex="1">
> >+            <xul:image class="error-image" xbl:inherits="type"/>
> >+          </xul:vbox>
> >+        </xul:hbox>
> 
> One of the two "image-box"s ought to be enough.

I need it centered both ways (vertical and horizontal) but I could probably do it on one box with styles...
> >+          <xul:vbox>
> >+            <xul:label class="console-message"
> >+                       xbl:inherits="type,xbl:text=msg"/>
> >+          </xul:vbox>
> 
> Any reason for the vbox around the label?

Makes room for large error messages, instead of extending horizontally, it will extend vertically first.
> >+          <xul:vbox class="error-source">
> >+            <xul:hbox>
> 
> Again: The hbox should be sufficient.

Not really, this has to extend as much as *possible* while not creating scrollbars. This was one of my goals with the redesign, in the old console long sources or long urls could cause horizontal scrollbars. This setup prevents that.
> >+              <xul:vbox flex="1">
> >+                <xul:label class="console-source" anonid="error-source-start"
> >+                           crop="start" xbl:inherits="value=errorSrcStart"/>
> >+                <xul:hbox>
> >+                  <xul:label class="console-dots" crop="start" flex="1"/>
> >+                  <xul:box class="error-caret"/>
> >+                </xul:hbox>
> >+              </xul:vbox>
> >+              <xul:vbox flex="1">
> >+                <xul:label class="console-source" anonid="error-source-end"
> >+                           crop="end" flex="1" xbl:inherits="value=errorSrcEnd"/>
> >+              </xul:vbox>
> 
> The original console achieved this through pure CSS. Is this no longer
> possible? Note: Code should continue to be set in a monospaced font.

I'm not sure I understand this, the old console used the repeatChar() hack to get the necessary amount of cols on the error-dots label. I completely removed that. The new error-dots is part of the same hbox as errorSrcStart, which ends at the column of the error, so the error-dots and error-caret are automatically in the correct place. Additionally this allows for the best cropping, it will crop the beginning of errorSrcStart and the end of errorSrcEnd so that the column will always be visible without needing to scroll. it works out very well in my tests.
> >+      <field name="errorMsg">
> >+        this.getAttribute("msg");
> >       </field>
> 
> Shouldn't this be a property, so that the value is dynamically updated when the
> attribute changes? The same also applies to href, lineNumber, preLine, preCol,
> colNumber and errorSrc. Then again, are all of these needed at all?

These attributes never change anyhow.


I guess I should probably be commenting a lot more on the code/css/xul as to what they all do, why they're needed, etc. I'll address the rest of your comments and add a bunch of commenting where necessary next.
Blocks: 490499
Depends on: 490178
No longer depends on: 490499

Comment 11

8 years ago
(In reply to comment #10)
> I really use it a lot more in the full patch

Indeed, I noticed when reviewing part II of your patches. As mentioned over there, what you want here is a CONSTANT.

> Because on Windows '\n' results in no lines at all :(

Works for me on the 1.9.1 branch. Could this be a regression on Trunk?

> Additionally this allows for the best cropping, it will crop the beginning
> of errorSrcStart and the end of errorSrcEnd

That's the bit I've missed. Right, let's keep it your way.

> These attributes never change anyhow.

... unless an extension tries to be clever. ;-) Fields and attributes can get out of sync quite quickly, so unless there's a significant performance penalty, we should play it safe here and not cause potential head-aches to somebody attempting to extend the Error Console.
Created attachment 375503 [details] [diff] [review]
part 1: move to richlistbox ver. 2

Updated per the comments. I didn't remove the hbox/vbox around the image as I found it necessary to accomplish what I wanted to do. Since the outer box must flex (to ensure the richlistitem spans the whole width of the richlistbox) the inner box directly beneath it for the image cannot flex, else the image is stretched more than necessary.

I also ifdef'ed the "\r\n" code. As to the WFM you mentioned, it works in firefox, I believe there's some sort of check for this in the code somewhere, but pasting into Notepad doesn't work (i.e. no lines show). This is true with the current error console.
Attachment #375295 - Attachment is obsolete: true
Attachment #375503 - Flags: review?(zeniko)
...oh and the keyset is used to define the ctrl-c and ctrl-a keys for the selection+copy commands.

Comment 14

8 years ago
Comment on attachment 375503 [details] [diff] [review]
part 1: move to richlistbox ver. 2

Looking good. A second pass:

>+#console:not([showchrome="true"]) > richlistitem[ischrome="true"] {

Note: showchrome and ischrome will have to be renamed should we ever want to allow to filter out content related errors as well. You can leave them for now, though.

>+const kChromePref = "showInConsole"; // pref to show/hide chrome:// errors

Nit: We prefer UPPERCASE names for constants in JavaScript.

>+  clearConsole: function console__clearConsole(aNotify) {
>+    if (aNotify) {
>+      // Clear the console and notify the service with a null message
>+      this.consoleService.logStringMessage(null);
>+      this.consoleService.reset();
>+    }
>+    // this clear came from the service, no need to re-notify

Nit: This comment only applies to the (inexistent) else clause.

>+  processMessage: function console__processMessage(aMessage) {
>+    // See if it's a script error to get more info
>+    if (aMessage instanceof Ci.nsIScriptError) {
>+      // filter chrome urls
>+        this.appendItem(aMessage, "error");
>+    }

Nit: The comment no longer applies and the indentation is wrong.

>+      var isChrome = (aItem.sourceName.substr(0, 9) == "chrome://");
>+      item.setAttribute("ischrome", isChrome);

We usually set these attributes conditionally (i.e. only when their value is "true").

>+#ifdef XP_WIN
>+    var newLine = "\r\n";
>+#else
>+    var newLine = "\n";
>+#endif

Please also make this a constant (so that you don't need two ifdefs for the same issue).

>+    if (type == "error")
>+      itemText.push(this.bundle.getString("typeError") + " ");
>+    else if (type == "warning")
>+      itemText.push(this.bundle.getString("typeWarning") + " ");
>+    itemText.push(aItem.errorMsg);

If you make the typeError/typeWarning string a prefix and then push |prefix + aItem.errorMsg|, you won't have to explicitly push any newlines below and can instead join with it. That'll make things easier to read and won't have anybody wonder about:

>+    if (type != "message")
>+      itemText.push(newLine);

>+    for (var i = 0; i < items.length; i++) {
>+      if (mode == "all" || items[i].getAttribute("type") == mode) {
>+        textToCopy.push(this.getItemText(items[i]));
>+      }
>+    }

AFAICT this doesn't do the right thing WRT chrome errors: Select a few errors including a chrome one, then hide chrome errors and then hit Ctrl+C. Why not check for the item's visibility instead?

>+  QueryInterface: function(aIID) {
>+   if (aIID.equals(Components.interfaces.nsIController) ||
>+       aIID.equals(Components.interfaces.nsISupports))
>+     return this;

Nit (again): Missing braces after multi-line condition.

>-        <toolbarbutton type="radio" group="mode" id="Console:modeAll" 
>+        <toolbarbutton type="radio" group="mode" id="console-all"

Please keep "mode" in the id. "console-all" isn't ideally descriptive.

>+      <field name="linkLabel" readonly="true">

Despite what parts of our code seem to imply, there are no readonly fields. Just drop the attribute.

>+      <method name="setErrorSrc">
>+        <parameter name="errorSrcStart"/>
>+        <parameter name="errorSrcEnd"/>

Do we really need this setter? If so, I'd prefer the parameters to be "errorSrc" and "errorColumn" and then do the splitting as you already do it above in console.js.

>+/* By default a richlistbox has margins all around, we only want a margin on top */

Why? ;-)

>+/* For visual consistency sake, make each richlistitem at least 45px tall so as

Nit: for consistency_'s_ sake.

>+..console-message[type="message"] {

Nit (again): double dot.

Also: Am I correct in my interpretation that code is no longer presented in a monospace font? If so, please change that back here (unrelated change).
Attachment #375503 - Flags: review?(zeniko)
(In reply to comment #14)
> >+/* By default a richlistbox has margins all around, we only want a margin on top */
> 
> Why? ;-)

It makes for less room all around unnecessarily. IMO, it also doesn't look nice in the error console, do you want me to remove it or just comment about this in the css?

Note: new patch is ready, just waiting on this last comment.
Oh and fyi, I checked in DOMi in the accessibility tree and it does skip visibility:collapse items.

Comment 17

8 years ago
(In reply to comment #15)
> It makes for less room all around unnecessarily.

Sorry for not being clearer. The question was: Why do we want to keep the margin above the richlistbox? AFAICT the current Error Console's list doesn't have any margin at all, so it seems like "margin: 0px" and no comment should just do.
Created attachment 375537 [details] [diff] [review]
part 1: move to richlistbox ver. 2.1

Nits addressed. I also removed the margin completely.
Attachment #375503 - Attachment is obsolete: true
Attachment #375537 - Flags: review?(zeniko)
Created attachment 375538 [details] [diff] [review]
part 1: move to richlistbox ver. 2.1

Sorry, some crap from another patch leaked into this one, same thing minus the image loading stuff :/
Attachment #375537 - Attachment is obsolete: true
Attachment #375538 - Flags: review?(zeniko)
Attachment #375537 - Flags: review?(zeniko)

Comment 20

8 years ago
Comment on attachment 375538 [details] [diff] [review]
part 1: move to richlistbox ver. 2.1

This is likely the last review round needed by myself. With the following fixed, this'd be r+=me. You'll then need a Toolkit peer's review.

>+// On Windows, some text editors (eg Notepad) expect \r\n for the end of the line
>+// marker.

Nit: This comment should be preprocessed away (i.e. replace the // with #), as it applies to the #ifdef and not the constant.

>+  get showChrome() {
>+    delete this.showChrome;
>+    return this.showChrome =
>+      this.prefBranch.getBoolPref(CHROME_PREF);

Prefs usually don't need to be cached. And you will need this one so infrequently, that it'll be easier to just get it in place (i.e. replace |this.showChrome| with |this.prefBranch.getBoolPref(CHROME_PREF)| wherever needed in this and the follow-up patch).

>+  get console() document.getElementById("console"),
>+
>+  get bundle() document.getElementById("ConsoleBundle"),

Any reason for not caching these two? Especially considering that you'll use both of them in a loop later on.

>+  clearConsole: function console__clearConsole(aNotify) {
>+    // Clear the console and notify the service with a null message. Since at
>+    // times we clear the console, make sure we haven't notified the service
>+    // ourselves yet.

This comment isn't really accurate: 1) We don't always notify the service and 2) we don't make sure we haven't notified the service inside this function.

>+  appendItem: function console__appendItem(aItem, aType) {

Nit: aItem is an unfortunate name, considering that you'll create the item _inside_ the function. Maybe rather aText or aMessage?

>+    else if (aType == "message") {
>+      item.setAttribute("type", "message");
>+      item.setAttribute("msg", aItem);
>+    }

What about aType == "object"? The previous implementation also appended it as if it had been aType == "message". Please continue to do that (you currently get a blank item).

>+  getItemText: function console__getItemText(aItem) {

For readability, this function might benefit of a few blank lines to give it some structure.

>+      itemText.push(this.bundle.getFormattedString("errLineCol", [aItem.lineNumber,
>+                                                   aItem.colNumber]));

Nit: Both aItem.s should be aligned.

>+  copySelectedItems: function console__copySelectedItems() {
>+#ifdef XP_WIN
>+    var separator = NEW_LINE + NEW_LINE;

Nits: Drop the #ifdef (this file shouldn't really pass the preprocessor in this state) and move the separator right before where it's needed.

>+    var items = this.console.selectedItems;
>+    var mode = this.console.getAttribute("mode");

Nit: Unused variable.

>     // or could use appendMessage which doesn't persist

Nit: appendMessage doesn't exist anymore.

>+  <keyset id="editMenuKeys"/>

This either belongs into a follow-up patch or misses actual <key> children.

>+      <menuitem id="menu_copy"/>
>+      <menuitem id="menu_selectAll"/>

Don't these miss a label, accesskey and command id?

>+      <method name="setErrorSrc">

>+          return errorSrc;

This return value doesn't really make sense and it's not needed, either (this is a method and no property, after all).

>+      <property name="errorMsg"
>+                onget="return this.getAttribute('msg');"
>+                onset="this.setAttribute('msg', val); return val;"/>
>+  </implementation>

Nit: </implementation> isn't properly indented.

>-@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");

Deleted too much (repeatedly)?

>-/* ::::: row colors ::::: */
>-
>-.console-row[type="error"],
>-.console-row[type="exception"] {
>-  background-color: #FFD0DC;
>-}
>-
>-.console-row[type="warning"] {
>-  background-color: #F8F3CC;
>-}
>-
>-.console-row[type="message"] {
>-  background-color: #D3EDFF;
>-}

We'll want to keep these, at least in this bug.

>+#console-mode-warning:hover, #console-mode-warning[checked="true"]  {
>+  -moz-image-region: rect(24px, 72px, 48px, 48px)

Nit: :hover and [checked="true"] used to be on two lines. Please keep it that way for better readability.
Attachment #375538 - Flags: review?(zeniko) → review-
(In reply to comment #20)
> This is likely the last review round needed by myself. With the following
> fixed, this'd be r+=me. You'll then need a Toolkit peer's review.

Thanks!

> >+  clearConsole: function console__clearConsole(aNotify) {
> >+    // Clear the console and notify the service with a null message. Since at
> >+    // times we clear the console, make sure we haven't notified the service
> >+    // ourselves yet.
> 
> This comment isn't really accurate: 1) We don't always notify the service and
> 2) we don't make sure we haven't notified the service inside this function.

What I meant is that we shouldn't re-notify the service if we've notified already, which is indicated by the parameter, I'll restructure this comment.

> >+  copySelectedItems: function console__copySelectedItems() {
> >+#ifdef XP_WIN
> >+    var separator = NEW_LINE + NEW_LINE;
> 
> Nits: Drop the #ifdef (this file shouldn't really pass the preprocessor in this
> state) and move the separator right before where it's needed.

It actually did work, but probably because I was on XP_WIN :)

> >+  <keyset id="editMenuKeys"/>
> 
> This either belongs into a follow-up patch or misses actual <key> children.

The keyset as well as the commandset come from editMenuOverlay.xul. AFAIK the way to set them on your context menus and to enable the keys is by putting this "place holder" in the document without any actual keys or commands, since they are specified in the overlay. Am I missing something?
> >+      <menuitem id="menu_copy"/>
> >+      <menuitem id="menu_selectAll"/>
> 
> Don't these miss a label, accesskey and command id?

No they're "inherited" from the editMenuOverlay.xul context menu and are really only place holders.
> >+      <method name="setErrorSrc">
> 
> >+          return errorSrc;
> 
> This return value doesn't really make sense and it's not needed, either (this
> is a method and no property, after all).

I'll fix this, I was aiming for consistency (since we do that on every other set) but this is really different like you said.
> >-@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> 
> Deleted too much (repeatedly)?

Yes. I'm not really sure what this is for, but I'll put them back. (winstripe doesn't have this, should I add it there?)
> >-/* ::::: row colors ::::: */
> >-
> >-.console-row[type="error"],
> >-.console-row[type="exception"] {
> >-  background-color: #FFD0DC;
> >-}
> >-
> >-.console-row[type="warning"] {
> >-  background-color: #F8F3CC;
> >-}
> >-
> >-.console-row[type="message"] {
> >-  background-color: #D3EDFF;
> >-}
> 
> We'll want to keep these, at least in this bug.

You want to override the default richlistitem look? What for?

Comment 22

8 years ago
(In reply to comment #21)
> > >-@namespace url("http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul");
> Yes. I'm not really sure what this is for, but I'll put them back. (winstripe
> doesn't have this, should I add it there?)

Pretty much every stylesheet should start with these (so that it's obvious that the following rules only apply to elements in the XUL namespace).

> You want to override the default richlistitem look? What for?

You'll have to ask the Pinstripe authors. Anyway, this isn't the place to revisit that decision.
Created attachment 375916 [details] [diff] [review]
part 1: move to richlistbox ver. 2.2

This still depends on bug 490178.

I didn't remove the commandset or keyset because I think they are correct, let me know what I should do if this is wrong.
Attachment #375538 - Attachment is obsolete: true
Attachment #375916 - Flags: review?(gavin.sharp)
Comment on attachment 375916 [details] [diff] [review]
part 1: move to richlistbox ver. 2.2

I'm unlikely to be able to review such a large patch in the near future, unfortunately. Perhaps one of the Neil's would be a better reviewer.

Are all of these changes really interdependent such that they can't be made progressively?
Comment on attachment 375916 [details] [diff] [review]
part 1: move to richlistbox ver. 2.2

->over to neil then.

The original patch has already been split up as much as it can be. All of this has to do with the richlistbox change.
Attachment #375916 - Flags: review?(gavin.sharp) → review?(neil)

Comment 26

8 years ago
(In reply to comment #25)
You might be able to split it up further by first refactoring as much from consoleBindings.xml into console.js (as much copy&pasting as possible without changing the code); in a second step do as little as possible to change to using a richlistbox; and in a third step clean things up to get into the state of the patch you've posted.
Even then it would help if you didn't change all the classes and ids around...
Comment on attachment 375916 [details] [diff] [review]
part 1: move to richlistbox ver. 2.2

Just an update: I am currently waiting for review on bug 490178 in order to proceed with this one, I'll try to break up the patches as well. Removing review request.
Attachment #375916 - Attachment is obsolete: true
Attachment #375916 - Flags: review?(neil)
Blocks: 120680
Blocks: 118171
Blocks: 67603
Duplicate of this bug: 418598

Updated

8 years ago
Blocks: 418598

Updated

8 years ago
Component: Disability Access → Error Console
Product: Firefox → Toolkit
QA Contact: disability.access → error.console
Bug 426727 is needed to make the selection background work with the foreground colors.
Depends on: 426727
Created attachment 394105 [details] [diff] [review]
patch ver. 2.3

Updated to trunk, removed some unnecessary noise. I'm gonna try to break this up tonight, I'll see what I can do...
No longer depends on: 426727
Depends on: 426727
Created attachment 396249 [details] [diff] [review]
patch ver. 2.3

Ok, bug 490178 is fixed, we can get this started now! I haven't really found a way to break this up more, or even get considerably less noise, as 1) I've only changed class names where they were irrelevant or where they changed considerably anyhow, and 2) all of these changes are necessary to get to the state of a richlistbox. I could simply upload three patches, one for style differences, one for xul/xbl differences and the third for the js differences, but I think that'd make it even more obscure. It's really only a 50-60k patch...

The only change since the previous patch is s/visibility: collapse/display: none for the richlistitems, due to bug 510824.

Let me know if you're ok with the above.
Attachment #394105 - Attachment is obsolete: true
Attachment #396249 - Flags: review?(neil)
Neil: ping? Would be nice to get this for 1.9.3 if possible...

Updated

8 years ago
Blocks: 490886
No longer depends on: 490886
Created attachment 405315 [details] [diff] [review]
patch ver 2.3a

I wanted to see if this patch helped with a vaguely-related problem I was having, and I had to adjust it for changes to toolkit/themes/pinstripe/global/console/console.css, so here is the rediffed version.  There are no changes other than to that file.

I noted some minor problems in the course of adjusting the patch:

 - @namespace rules must appear *after* @import rules (but before everything else).  Fixed this in the file I updated, but may need fixing elsewhere.
 - IMO it is better to write "margin: 0;" than "margin: 0px;" (and similarly for other zero lengths).
 - Is the .console-caret rule still used?

I also have some aesthetic suggestions about the new error console, which will be much easier to explain if I attach an annotated screen shot, so I'll put that in a separate message.
Attachment #396249 - Attachment is obsolete: true
Attachment #405315 - Flags: review?(neil)
Attachment #396249 - Flags: review?(neil)
Created attachment 405318 [details]
annotated error console screenshot

Here's a screen shot of the as-patched error console with my aesthetic suggestions noted.  Please let me know if the text is illegible, I think it shrank in the image export.

Comment 36

8 years ago
Some of us are not as young as we once were, so yes the text is basically illegible.
Pushing out old bugs that I won't have time to actually get to. Feel free to use/extend any patches attached...
Assignee: highmind63 → nobody
Status: ASSIGNED → NEW
we need a match.com service for bugzilla patches/bugs :)
Whiteboard: [patchlove][has draft patch][needs new assignee]
No longer blocks: 490499
No longer blocks: 67603
Is there *any* chance on Earth we can ever get this fixed? This bug has been lying around forever, and the Error console is one of the most inaccessible parts of Firefox.
We're actually looking to get rid of the error console (bug 602006) - the only thing holding that up is that it's the only place some errors appear.
Comment on attachment 405315 [details] [diff] [review]
patch ver 2.3a

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

::: toolkit/locales/en-US/chrome/global/console.properties
@@ +2,5 @@
>  typeWarning=Warning:
>  errFile=Source File: %S
>  errLine=Line: %S
>  errLineCol=Line: %S, Column: %S
> +errCode=Source Code: %S

In case this still gets fixed, this will need a key change here.
(In reply to Marco Zehe (:MarcoZ) from comment #39)
> Is there *any* chance on Earth we can ever get this fixed? This bug has been
> lying around forever, and the Error console is one of the most inaccessible
> parts of Firefox.

marcoz, does the new webconsole resolve this area for you?

Comment 43

4 years ago
I (as a screen reader user) do use the Web Console and find it works fairly well. However, a few things would make it better (especially for less experienced screen reader users):
1. Provide a way to move focus to the output list with the keyboard. At least, I haven't found a way. The list is focusable, but you can't tab or shift+tab to it.
2. A quicker way to return focus to the Web Console after moving elsewhere. Currently, the quickest way is to press control+l and shift+tab three times. Perhaps it could be included in the panels accessible via f6.
3. Allow the user to tab/shift+tab to the toolbar buttons (but only one tab stop for the buttons; left/right arrow moves between buttons).
4. The accessible names of the list items should include the message content. Currently, they only include the time and location. (The message is currently exposed as a text leaf child accessible.)
5. Remove the location links from the tab order or at least only include the link for the focused output item.

Comment 44

4 years ago
Also, the way autocomplete is handled currently is pretty ugly for screen readers. It is communicated as an alert, so screen readers try to read the entire autocomplete list. I'm not quite sure how to best handle this, but to be honest, I'd prefer autocomplete being inaccessible altogether to the current situation.
Comment on attachment 405315 [details] [diff] [review]
patch ver 2.3a

Somehow I suspect the r? on this is moot by now.
Attachment #405315 - Flags: review?(neil)

Comment 46

3 years ago
Created attachment 8474467 [details] [diff] [review]
WIP patch

This patch:
* makes the console output a live region (except for input from the user);
* stops the autocomplete from being treated as an alert for accessibility; and
* focuses the selected autocomplete item for accessibility.

Marco, I'd appreciate feedback in general. In addition:
1. What are your thoughts on the output area? I guess it'd be super nice if the items were focusable list items, but I'm not sure it's strictly necessary, as you can get at these easily with, say, NVDA object navigation. (I'm partially tempted to give it role="document" so you can just read it with, say, NVDA browse mode. This seems a bit extreme, though.)
2. Should the links in the output area be tabbable? This doesn't make sense to me if the items themselves aren't, but the question is whether this will break keyboard users who aren't also screen reader users. I doubt it, since right now, they can't interact with the items in any other way except for the links.
3. Even aside from (1) and (2), I think the live region and autocomplete stuff is a huge improvement already. Should we split this into multiple bugs?
Attachment #8474467 - Flags: feedback?(marco.zehe)

Comment 47

3 years ago
Created attachment 8474478 [details] [diff] [review]
WIP patch... that isn't broken

Sorry. First patch was broken.

The bit which is supposed to stop input from the user being echoed isn't working yet. I'll keep digging. Nevertheless, the rest works.

Same feedback questions.
Attachment #8474467 - Attachment is obsolete: true
Attachment #8474467 - Flags: feedback?(marco.zehe)
Attachment #8474478 - Flags: feedback?(marco.zehe)
I believe this patch is better suited for bug 977267. You're working on the web console, not on the browser console, formerly known as error console. :)
Summary: Make Error Console work better with screen readers → Make browser Console work better with screen readers

Comment 49

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #48)
> I believe this patch is better suited for bug 977267. You're working on the
> web console, not on the browser console, formerly known as error console. :)
Sorry; I was confused by comment 42. Nevertheless, I suspected (and have just confirmed) that these two consoles both use the same code, so this patch applies to both. However, I could split the live region stuff into the other bug.
Comment on attachment 8474478 [details] [diff] [review]
WIP patch... that isn't broken

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

f=me. This improves interaction with autocompletes and the console in general by a big magnitude. One question remains.

::: browser/devtools/webconsole/webconsole.xul
@@ +173,5 @@
>                   placeholder="&filterOutput.placeholder;" tabindex="2"/>
>        </toolbar>
>  
>        <hbox id="output-wrapper" flex="1" context="output-contextmenu" tooltip="aHTMLTooltip">
> +        <div xmlns="http://www.w3.org/1999/xhtml" id="output-container" tabindex="1" role="log" />

Tabindex > 0 puts this at the very front of the tab order. Does it make sense to keep it here, or should that tabindex be 0 so it inlines with the rest of the controls?
Attachment #8474478 - Flags: feedback?(marco.zehe) → feedback+

Comment 51

3 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #50)
> Tabindex > 0 puts this at the very front of the tab order. Does it make
> sense to keep it here, or should that tabindex be 0 so it inlines with the
> rest of the controls?
I didn't change this. I can't be certain as to the original intent, but it looks quite intentional based on the other tabindex values. I'm guessing the idea is that the output control should come before the output filtering toolbar. However, this falls apart a bit because the links from the output area have no tabindex, so they end up *after* the toolbar. For this reason, I'd personally prefer to set tabindex="0" here as you suggest.
(In reply to James Teh [:Jamie] from comment #51)
> (In reply to Marco Zehe (:MarcoZ) from comment #50)
> > Tabindex > 0 puts this at the very front of the tab order. Does it make
> > sense to keep it here, or should that tabindex be 0 so it inlines with the
> > rest of the controls?
> I didn't change this. I can't be certain as to the original intent, but it
> looks quite intentional based on the other tabindex values. I'm guessing the
> idea is that the output control should come before the output filtering
> toolbar. However, this falls apart a bit because the links from the output
> area have no tabindex, so they end up *after* the toolbar. For this reason,
> I'd personally prefer to set tabindex="0" here as you suggest.

Yes, I think it makes sense to first filter things and then tab into the remaining output. So I'd suggest to add this when you ask a peer for review.

Comment 53

3 years ago
Created attachment 8475789 [details] [diff] [review]
Patch

Updated patch:
* Removes tabindex="0" on the output area so it at least ends up in the same spot as its links.
* Gives the output area role="document" so ATs will treat it like a document.
* Corrects the code to set aria-live="off" on input from the user so it isn't reported automatically. Note that this won't currently work with NVDA due to a bug in NVDA, but you can confirm it by poking at the attributes on a piece of input.
Assignee: nobody → jamie
Attachment #405315 - Attachment is obsolete: true
Attachment #8474478 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8475789 - Flags: feedback?(marco.zehe)

Comment 54

3 years ago
(In reply to James Teh [:Jamie] from comment #49)
> (In reply to Marco Zehe (:MarcoZ) from comment #48)
> > I believe this patch is better suited for bug 977267. You're working on the
> > web console, not on the browser console, formerly known as error console. :)
> Sorry; I was confused by comment 42. Nevertheless, I suspected (and have
> just confirmed) that these two consoles both use the same code, so this
> patch applies to both. However, I could split the live region stuff into the
> other bug.

Excuse me, this bug is for the toolkit Error Console still used by Thunderbird, SeaMonkey, and other xulrunner applications. See:
http://mxr.mozilla.org/mozilla-central/find?string=console%2Fcontent%2F&tree=mozilla-central&hint=

Please don't hijack bugs. Instead file new bugs in the right product (Firefox) and category (dev tools). Thanks.
Status: ASSIGNED → NEW

Updated

3 years ago
Attachment #8475789 - Attachment is obsolete: true
Attachment #8475789 - Flags: feedback?(marco.zehe)

Updated

3 years ago
Assignee: jamie → nobody
Summary: Make browser Console work better with screen readers → Make Error Console work better with screen readers

Comment 55

3 years ago
(In reply to Philip Chee from comment #54)
> Excuse me, this bug is for the toolkit Error Console still used by
> Thunderbird, SeaMonkey, and other xulrunner applications.
My sincerest apologies. This is rather confusing due to comment 42 and the fact that Firefox has replaced both consoles altogether, among other things.

> Please don't hijack bugs. Instead file new bugs in the right product
> (Firefox) and category (dev tools). Thanks.
I did ask Marco's opinion regarding hijacking this. I didn't realise this console was still in use. Sorry again.
(In reply to James Teh [:Jamie] from comment #55)
> (In reply to Philip Chee from comment #54)
> > Excuse me, this bug is for the toolkit Error Console still used by
> > Thunderbird, SeaMonkey, and other xulrunner applications.
> My sincerest apologies. This is rather confusing due to comment 42 and the
> fact that Firefox has replaced both consoles altogether, among other things.
> 
> > Please don't hijack bugs. Instead file new bugs in the right product
> > (Firefox) and category (dev tools). Thanks.
> I did ask Marco's opinion regarding hijacking this. I didn't realise this
> console was still in use. Sorry again.

My sincerest apologies as well, I was suffering from the same points of confusion Jamie voiced. Happens even to "old rabbits". :)
The Error Console has been removed in favor of the Browser Console (see Bug 1278368), and the component is going to be removed.  If this bug is also relevant in the Browser Console, please reopen and move this into Firefox -> Developer Tools: Console.
Status: NEW → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → WONTFIX

Comment 58

10 months ago
I am mass-reopening and re-componenting every single one of the Toolkit:Error Console bugs that appear to have been closed without anyone even *glancing* at whether they were relevant to the Browser Console.

If you want to close a whole bunch of old bugs -- FOR ANY REASON -- it is YOUR RESPONSIBILITY to check EVERY SINGLE ONE OF THEM and make sure they are no longer valid.  Do not push that work onto the bug reporters.

(It's okay to close bugs that haven't been touched in years when they don't have enough information for you to figure out whether the problem is still relevant to the current software - the reporter probably isn't coming back to clarify.  But that is the ONLY situation where it is okay.)

(I'm going to have to do this in two steps because of the way the "change several bugs at once" form works.  Apologies for the extra bugspam.)
Status: RESOLVED → REOPENED
Component: Error Console → Developer Tools: Console
Product: Toolkit → Firefox
Resolution: WONTFIX → ---
Bug doesn't translate to browser console, so resolving
Status: REOPENED → RESOLVED
Last Resolved: 10 months ago10 months ago
Component: Developer Tools: Console → Error Console
Product: Firefox → Toolkit
Resolution: --- → WONTFIX
(Assignee)

Updated

10 months ago
Product: Toolkit → Toolkit Graveyard
You need to log in before you can comment on or make changes to this bug.