Closed Bug 1210778 Opened 9 years ago Closed 8 years ago

Make about:debugging more accessible.

Categories

(DevTools :: about:debugging, defect, P2)

defect

Tracking

(firefox48 fixed)

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: janx, Assigned: yzen)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

As mentioned in bug 1196785 comment 75, the about:debugging page does not work well for RTL users. It would also be great to make sure it's accessible for all our users.
Alexander, do you know who I could ask for an accessibility review? I'd like to know all the accessibility problems that need to be fixed in the new "about:debugging" page.
Flags: needinfo?(surkov.alexander)
Component: Developer Tools → Developer Tools: about:debugging
Marco's experience should be definitely helpful here.
Flags: needinfo?(surkov.alexander) → needinfo?(mzehe)
1. Make sure all actionable controls are also keyboard focusable. I am thinking of the "Add-Ons" and "Workers" tabs at the top, for example. Tab does not jump to these, and there is no way to activate them with the keyboard.
2. Make sure focus is always visible when tabbing through the page.
3. Test the page with a screen reader such as NVDA under Windows to make sure you hear everything you expect to hear. The current way of just having a cillion buttons labeled "Debug" is, for example, counter-intuitive when tabbing through.
Flags: needinfo?(mzehe)
Thanks a lot Marco! I'm hoping to address this in the coming months.
(In reply to Jan Keromnes [:janx] from comment #4)
> Thanks a lot Marco! I'm hoping to address this in the coming months.
Very cool! I'll be here to help if you need advice or testing! :-)
Yura, this is the bug I told you about.
Flags: needinfo?(yzenevich)
Assignee: nobody → yzenevich
Status: NEW → ASSIGNED
Flags: needinfo?(yzenevich)
Attached patch 1210778 a11y patch (obsolete) — Splinter Review
Attachment #8699502 - Flags: review?(eitan)
Attached patch 1210778 dev tools patch (obsolete) — Splinter Review
Attachment #8699503 - Flags: review?(janx)
Jan, not that you need both applied for testing to work.
Comment on attachment 8699503 [details] [diff] [review]
1210778 dev tools patch

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

Very cool, thanks a lot Yura! Everything seems to work fine, my in-line remarks are mostly nits.

Alex, I'd love to know what you think about the new python marionette test. Is this a good example to follow for other dev tools?

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +71,5 @@
> +
> +input[type="checkbox"]:hover + label:before,
> +input[type="checkbox"]:focus + label:before {
> +  border-color: var(--in-content-border-focus);
> +}

Nice tricks with the checkboxes!

Suggestion: Maybe they could also benefit about:addons and about:preferences? The common file also imported by about:debugging is toolkit/themes/shared/in-content/common.inc.css (as an example, I changed a few checkbox rules in https://hg.mozilla.org/integration/fx-team/rev/3639f5216ac0 )

Nit: However, if you still want to add these new `input[type="checkbox"]` rules to aboutdebugging.css, please move them to the top of the `/* Prefs */` section (they're not really related to `/* Targets */`).

Also, once you've focused a checkbox, how do you toggle it from the keyboard? I've tried 'Enter' and 'x', without success.

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +41,5 @@
>          let value = element.getAttribute("value");
> +        element.addEventListener("click", () =>
> +          location.hash = "#" + value);
> +        element.addEventListener("keyup", e => {
> +          if (e.key === " " || e.key === "Enter") {

Nit: Please rename this attribute `event` instead of single-char `e`.

@@ +118,5 @@
>      this.client.connect(() => {
>        // Show the first available tab.
>        this.showTab();
> +      window.addEventListener("hashchange", () =>
> +        this.showTab(location.hash.substr(1)));

Nit: No need to change this.

::: devtools/client/aboutdebugging/aboutdebugging.xhtml
@@ +19,5 @@
>      <script type="application/javascript;version=1.8" src="chrome://devtools/content/aboutdebugging/aboutdebugging.js"></script>
>    </head>
>    <body id="body">
> +    <div id="categories" role="tablist">
> +      <div role="tab" class="category" value="addons" tabindex="0" aria-selected="true" selected="true" aria-controls="tab-addons">

Nit: Please move the `role` attribute after the `class`.

@@ +20,5 @@
>    </head>
>    <body id="body">
> +    <div id="categories" role="tablist">
> +      <div role="tab" class="category" value="addons" tabindex="0" aria-selected="true" selected="true" aria-controls="tab-addons">
> +        <img role="presentation" class="category-icon" src="chrome://devtools/skin/images/debugging-addons.svg"/>

Nit: Please move the `role` attribute after the `class`.

@@ +25,3 @@
>          <div class="category-name">&aboutDebugging.addons;</div>
>        </div>
> +      <div role="tab" class="category" value="workers" tabindex="0" aria-controls="tab-workers">

Nit: Please move the `role` attribute after the `class`.

@@ +25,4 @@
>          <div class="category-name">&aboutDebugging.addons;</div>
>        </div>
> +      <div role="tab" class="category" value="workers" tabindex="0" aria-controls="tab-workers">
> +        <img role="presentation" class="category-icon" src="chrome://devtools/skin/images/debugging-workers.svg"/>

Nit: Please move the `role` attribute after the `class`.

::: devtools/client/aboutdebugging/components/target-list.js
@@ +28,5 @@
>      return (
>        React.createElement("div", { id: this.props.id, className: "targets" },
> +        React.createElement("h2", null, this.props.name),
> +        targets.length > 0 ?
> +          React.createElement('ul', {className: "target-list"}, targets) :

Nit: Local coding style uses double quotes instead of single quotes.

Nit: Please add a space after "{" and another one before "}".

::: devtools/client/aboutdebugging/test/marionette/test_aboutdebugging.py
@@ +18,5 @@
> +        self.marionette.navigate('about:debugging')
> +        self.accessibility = Accessibility(self.marionette)
> +
> +    def test_addons(self):
> +        self.assertEqual(self.marionette.get_url(), 'about:debugging')

Nit: I think this checks the page before it's fully initialized, otherwise the URL would be 'about:debugging#addons'. Maybe this test should wait for the first hash change? (I assume it's not a problem if about:debugging is not entirely accessible before being fully initialized).
Attachment #8699503 - Flags: review?(janx)
Attachment #8699503 - Flags: review+
Attachment #8699503 - Flags: feedback?(poirot.alex)
Comment on attachment 8699503 [details] [diff] [review]
1210778 dev tools patch

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

Just a couple of comments and i'll gladly take care of the nits.

::: devtools/client/aboutdebugging/aboutdebugging.js
@@ +118,5 @@
>      this.client.connect(() => {
>        // Show the first available tab.
>        this.showTab();
> +      window.addEventListener("hashchange", () =>
> +        this.showTab(location.hash.substr(1)));

So the reason I did this is because the listener will be called twice, once on click and then on hashchange. I thought it'd be helpful to fix it so it happens once and in one place.

::: devtools/client/aboutdebugging/test/marionette/test_aboutdebugging.py
@@ +18,5 @@
> +        self.marionette.navigate('about:debugging')
> +        self.accessibility = Accessibility(self.marionette)
> +
> +    def test_addons(self):
> +        self.assertEqual(self.marionette.get_url(), 'about:debugging')

I think on the initial load it does not have a hash, until the click happens.
(In reply to Jan Keromnes [:janx] from comment #10)
> Also, once you've focused a checkbox, how do you toggle it from the
> keyboard? I've tried 'Enter' and 'x', without success.
The common, as in "universal", keystroke to toggle checkboxes on all platforms I know, including XUL, is the Space Bar. ;-)
Comment on attachment 8699503 [details] [diff] [review]
1210778 dev tools patch

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

Can't we have a mochitest browser for that? We already have some tests against about:debugging that you can reuse.
We would like to aim making it easier to run tests for devtools and using yet another test harness won't help.
Attachment #8699503 - Flags: feedback?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #13)
> Comment on attachment 8699503 [details] [diff] [review]
> 1210778 dev tools patch
> 
> Review of attachment 8699503 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can't we have a mochitest browser for that? We already have some tests
> against about:debugging that you can reuse.
> We would like to aim making it easier to run tests for devtools and using
> yet another test harness won't help.

The reason I wanted to use an existing integration testing framework to test user actions with devtools. It now also has good support for accessibility checks. The reason accessibility is ideally checked in integration is because it's very difficult otherwise to test whether something is truly accessible to users of assistive technology (such as unit tests). 

Also ateam is soon landing https://github.com/mozilla/firefox-ui-tests into m-c and it would provide an easy way to interact with browser components in integration tests.
Comment on attachment 8699502 [details] [diff] [review]
1210778 a11y patch

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

This is great! Did you check with somebody about including external modules and that we are doing that right?

Also, I would maybe not use the minified version. It seems like the tradeoff between debugging headache and any potential gains from minification may not be worth it.

Also, I think it would be good to have an update script that could automatically pull the latest axe into the tree. This will allow folks to keep this current. Check out:
https://dxr.mozilla.org/mozilla-central/source/dom/media/webvtt/update-webvtt.js
Attachment #8699502 - Flags: review?(eitan)
Hi Andreas,

just wanted to ni? you on this bug where I have an example of using marionette outside of the marionette tests. I can run it using 'mach marionette-test devtools/client/aboutdebugging/test/marionette/test_aboutdebugging.py' however I noticed if I want to make changes to marionette listener.js file to add logging, they are not taken into account. You mentioned looking at https://dxr.mozilla.org/mozilla-central/source/testing/marionette/client/marionette/runtests.py but I was hoping you have some more details.
Flags: needinfo?(ato)
(In reply to Yura Zenevich [:yzen] from comment #16)
> just wanted to ni? you on this bug where I have an example of using
> marionette outside of the marionette tests. I can run it using
> 'mach marionette-test devtools/client/aboutdebugging/test/marionette/test_aboutdebugging.py'
> however I noticed if I want to make changes to marionette listener.js file
> to add logging, they are not taken into account.

Changes to testing/marionette/*.js files should be picked up if tests are being run in-tree using mach.  All in-tree tests run against your locally built Firefox, which in turn symlinks (or copies, depending on your operating system) the resources defined in testing/marionette/manifest.jar.

Using the `logger` constants in Marionette, which uses Log.jsm, should appear in the stdout from Gecko.  You may redirect the stdout of Gecko to your shell by using the `--gecko-log -` flag as such:

    % ./mach marionette-test devtools/client/aboutdebugging/test/marionette/test_aboutdebugging.py --gecko-log -

Can you try do a clobber and fresh build?

    % ./mach clobber
    % ./mach build

Normally you should not have to rebuild after making changes in testing/marionette.
Flags: needinfo?(ato)
Priority: -- → P2
Hi Yura, thanks again for coming up with the original patch!

We will soon return to ensuring about:debugging is and remains accessible, so I wanted to check in on the status of this bug. Were you able to investigate how to run accessibility tests in JS browser mochitests? Would you like some help with this? This is something we'd like to do for the other devtools as well.

Also, a few conflicting patches landed in the meantime (sorry about that), but in bug 1245029 we tried to fix all the accessibility problems you identified here, so hopefully there won't be many left to fix.
Flags: needinfo?(yzenevich)
(In reply to Jan Keromnes [:janx] from comment #18)
> Hi Yura, thanks again for coming up with the original patch!
> 
> We will soon return to ensuring about:debugging is and remains accessible,
> so I wanted to check in on the status of this bug. Were you able to
> investigate how to run accessibility tests in JS browser mochitests? Would
> you like some help with this? This is something we'd like to do for the
> other devtools as well.
> 
> Also, a few conflicting patches landed in the meantime (sorry about that),
> but in bug 1245029 we tried to fix all the accessibility problems you
> identified here, so hopefully there won't be many left to fix.

Hi Jan, yes, I will try to get to this as soon as I land keyboard accessibility improvements to devtools im working on right now. In terms of a11y static testing, I should be able to use axe but within the browser test (no need for marionette). I would still like to use marionette though if possible to test user interactions as it has built in tools to test if things are accessible (but that could be a follow up).
Flags: needinfo?(yzenevich)
Comment on attachment 8699503 [details] [diff] [review]
1210778 dev tools patch

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

::: devtools/client/aboutdebugging/aboutdebugging.css
@@ +71,5 @@
> +
> +input[type="checkbox"]:hover + label:before,
> +input[type="checkbox"]:focus + label:before {
> +  border-color: var(--in-content-border-focus);
> +}

Space should do it (for form fields).
Attachment #8699502 - Attachment is obsolete: true
Attachment #8699503 - Attachment is obsolete: true
---
 devtools/client/aboutdebugging/aboutdebugging.css           |  5 +++++
 devtools/client/aboutdebugging/components/aboutdebugging.js | 12 +++++++-----
 devtools/client/aboutdebugging/components/addon-target.js   |  2 +-
 devtools/client/aboutdebugging/components/addons-tab.js     |  4 ++--
 devtools/client/aboutdebugging/components/tab-menu-entry.js | 11 ++++++++++-
 devtools/client/aboutdebugging/components/tab-menu.js       | 10 ++++++----
 devtools/client/aboutdebugging/components/target-list.js    |  4 ++--
 devtools/client/aboutdebugging/components/worker-target.js  |  2 +-
 devtools/client/aboutdebugging/components/workers-tab.js    |  4 ++--
 9 files changed, 36 insertions(+), 18 deletions(-)

Review commit: https://reviewboard.mozilla.org/r/46189/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46189/
Attachment #8741094 - Flags: review?(janx)
Comment on attachment 8741094 [details]
MozReview Request: Bug 1210778 - improving accessibility for about:debugging. r=janx

Thanks a lot Yura! Your changes make a lot of sense and the keyboard navigation works great!

R+, but it looks like we broke your patch by landing the first part of bug 1264063 (sorry about that!). It mostly just moved `render()` methods to the bottom of React classes though, so I hope the conflicts won't be too tedious to resolve.

Also, please check that the tests still work with the new DOM changes, e.g. by running about:debugging mochitests locally or by pushing to try.

>  const tabs = [{
> -  id: "addons",
> +  tabId: "addons",
> +  tabPanelId: "tab-addons",

Nit: I don't think we need to prefix these new items with "tab". Maybe keep "id" as is, and add a "panelId" item?
Attachment #8741094 - Flags: review?(janx) → review+
(In reply to Yura Zenevich [:yzen] from comment #19)
> In terms of a11y static testing, I should be able to use axe but within the browser test
> (no need for marionette). I would still like to use marionette though if
> possible to test user interactions as it has built in tools to test if
> things are accessible (but that could be a follow up).

Alex, I'm curious, what do you think about running marionette tests on about:debugging and the devtools, in order to test user interactions and see if some parts of the UI are inaccessible?
Flags: needinfo?(poirot.alex)
Comment on attachment 8741094 [details]
MozReview Request: Bug 1210778 - improving accessibility for about:debugging. r=janx

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46189/diff/1-2/
Attachment #8741094 - Flags: review+ → review?(janx)
Comment on attachment 8741094 [details]
MozReview Request: Bug 1210778 - improving accessibility for about:debugging. r=janx

Thanks again for improving about:debugging's accessibility! And for addressing my earlier comment.

However, some tests time out in https://treeherder.mozilla.org/#/jobs?repo=try&revision=eee5d77c7f41&selectedJob=19558100.

I believe this is due to the added <ul> in TargetList: Most tests are waiting for a mutation to happen on the ".targets" <div> and its children. However, this mutation now only happens when switching between 0 and 1 target (because this change causes the <ul> to be added/removed), but not when the number of targets changes (because this only mutates the <ul>'s children).

This makes waiting for mutation of a TargetList a little trickier, but not impossible. Maybe you can update the tests to wait for mutations on ".targets > ul" if there is one, or just on ".targets" if there is no <ul>?

There are currently 4 calls to `waitForMutation()` to be fixed for Add-ons, and 8 to be fixed for Service Workers[1] (look for things like `getElementById("addons")`, `getElementById("service-workers")` or `querySelector(".targets")`).

[0] https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Faboutdebugging%2Ftest%2F+waitForMutation(addon&redirect=false&case=false
[1] https://dxr.mozilla.org/mozilla-central/search?q=path%3Adevtools%2Fclient%2Faboutdebugging%2Ftest%2F+waitForMutation(serviceWorkersElement&redirect=false&case=false
Attachment #8741094 - Flags: review?(janx) → feedback+
Ah sorry I missed those :|
(In reply to Jan Keromnes [:janx] from comment #23)
> (In reply to Yura Zenevich [:yzen] from comment #19)
> > In terms of a11y static testing, I should be able to use axe but within the browser test
> > (no need for marionette). I would still like to use marionette though if
> > possible to test user interactions as it has built in tools to test if
> > things are accessible (but that could be a follow up).
> 
> Alex, I'm curious, what do you think about running marionette tests on
> about:debugging and the devtools, in order to test user interactions and see
> if some parts of the UI are inaccessible?

It would be interesting to know what is missing in mochitest-browser.
Overall it would be great to reduce the type of tests involved in devtools (we already have xpcshell, mochitest chrome and mochitest browser. And may be also some mochitest-plain :/).
I imagine the a11y tests could easily break if we tweak the DOM of the interface,
and so contributors would have to learn yet another new thing.
Flags: needinfo?(poirot.alex)
(In reply to Alexandre Poirot [:ochameau] from comment #28)
> (In reply to Jan Keromnes [:janx] from comment #23)
> > (In reply to Yura Zenevich [:yzen] from comment #19)
> > > In terms of a11y static testing, I should be able to use axe but within the browser test
> > > (no need for marionette). I would still like to use marionette though if
> > > possible to test user interactions as it has built in tools to test if
> > > things are accessible (but that could be a follow up).
> > 
> > Alex, I'm curious, what do you think about running marionette tests on
> > about:debugging and the devtools, in order to test user interactions and see
> > if some parts of the UI are inaccessible?
> 
> It would be interesting to know what is missing in mochitest-browser.
> Overall it would be great to reduce the type of tests involved in devtools
> (we already have xpcshell, mochitest chrome and mochitest browser. And may
> be also some mochitest-plain :/).
> I imagine the a11y tests could easily break if we tweak the DOM of the
> interface,
> and so contributors would have to learn yet another new thing.

So the main thing that is missing from the marionette tests is that every user interaction that is performed - click element, send keys to, is done with additional accessibility tests. This is because in many cases there are elements that are clickable or the ones that receive keys but are not actually accessible to screen readers for example. Similarly, every time marionette performs checks for visibility, DOM elements being invisible, selected and other state type checks, we also perform additional checks that they are in the same state for accessibility as well. I can take a look perhaps at possibly sharing the same additional checks between different test harnesses, but that would mean potentially affecting thousands of existing [browser chrome] tests. I was hoping, with firefox-ui tests picking up speed (https://dxr.mozilla.org/mozilla-central/source/testing/firefox-ui) I could use the same format too.
Comment on attachment 8741094 [details]
MozReview Request: Bug 1210778 - improving accessibility for about:debugging. r=janx

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46189/diff/2-3/
Attachment #8741094 - Flags: feedback+ → review?(janx)
Comment on attachment 8741094 [details]
MozReview Request: Bug 1210778 - improving accessibility for about:debugging. r=janx

LGTM, thanks!

> +/**
> + * Depending on whether there are addons installed, return either a target list
> + * element or it's container.

Nit: Typo on "its container".

> +function getAddonList(document) {
> +  return document.querySelector("#addons .target-list") ||
> +    document.querySelector("#addons .targets");
> +}

Thanks for fixing the add-on tests breakage!

However, the service worker tests also suffer from the same problem (it doesn't currently trigger, because there are no pre-existing service workers when the tests run, and we don't install more than one service worker). You don't have to fix this now, but please at least open a follow-up bug for it.
Attachment #8741094 - Flags: review?(janx) → review+
Comment on attachment 8741094 [details]
MozReview Request: Bug 1210778 - improving accessibility for about:debugging. r=janx

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46189/diff/3-4/
Attachment #8741094 - Flags: review+ → review?(janx)
Sorry, not sure how to carry over an r+. I updated sw tests and fixed the nit and pushed to try once more. I'll landed if try is good.
Comment on attachment 8741094 [details]
MozReview Request: Bug 1210778 - improving accessibility for about:debugging. r=janx

https://reviewboard.mozilla.org/r/46189/#review45145

(In reply to Yura Zenevich [:yzen] from comment #34)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=a05abd787fed

Try looks green! \o/

(In reply to Yura Zenevich [:yzen] from comment #35)
> Sorry, not sure how to carry over an r+. I updated sw tests and fixed the
> nit and pushed to try once more. I'll landed if try is good.

In general, you can carry over an r+ by editing attachment details, and setting the flag to r+ yourself. (That's for regular reviews, I'm not sure if MozReview has a special thing for that, but we don't need it anyway.)

However, in this case it's good that you re-flagged me for review, because I'd prefer that you don't add aboutdebugging-specific globals to the general devtools .eslintrc.

::: devtools/.eslintrc.mochitests:22
(Diff revision 4)
>      "TargetFactory": true,
> +    "uninstallAddon": true,
> +    "unregisterServiceWorker": true,
> +    "waitForInitialAddonList": true,
> +    "waitForMutation": true,
> +    "waitForServiceWorkerRegistered": true,

Nit: Please don't add these aboutdebugging-specific globals to the general devtools .eslintrc. Instead, please use a /* globals */ comment in each test file, or create an aboutdebugging-specific .eslintrc.

::: devtools/client/aboutdebugging/test/head.js:115
(Diff revision 4)
> + * @param  {DOMDocument}  document   #service-workers section container document
> + * @return {DOMNode}                 target list or container element
> + */
> +function getServiceWorkerList(document) {
> +  return document.querySelector("#service-workers .target-list") ||
> +    document.querySelector("#service-workers.targets");

Nit: For consistency, please add a space between "#service-workers" and ".targets", like in the "#addons .targets" query selector above.
Attachment #8741094 - Flags: review?(janx)
Yura, please address the two last nits, then feel free to carry over my r+ by setting it directly on the attachment and land the patch.
Flags: needinfo?(yzenevich)
Attachment #8741094 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/7849c9bb0832
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
(In reply to Carsten Book [:Tomcat] from comment #39)
> https://hg.mozilla.org/mozilla-central/rev/7849c9bb0832

How strange, that changeset has "author: Wes Kocher <wkocher@mozilla.com>", whereas it should have been "Yura Zenevich <yzenevich@mozilla.com>".

Accidental mistake or serious bug in our landing process? Should we fix it?
Flags: needinfo?(yzenevich)
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
I have no idea how that happened.
Flags: needinfo?(wkocher)
(In reply to Wes Kocher (:KWierso) from comment #41)
> I have no idea how that happened.

(In reply to Jan Keromnes [:janx] from comment #40)
> (In reply to Carsten Book [:Tomcat] from comment #39)
> > https://hg.mozilla.org/mozilla-central/rev/7849c9bb0832
> 
> How strange, that changeset has "author: Wes Kocher <wkocher@mozilla.com>",
> whereas it should have been "Yura Zenevich <yzenevich@mozilla.com>".
> 
> Accidental mistake or serious bug in our landing process? Should we fix it?

This is my mistake, I think I might have squashed an extra commit when building a patch and the author was altered. Apologies.
Flags: needinfo?(yzenevich)
if yura is fine with this i think its no big problem also since this has review and so :)
Flags: needinfo?(cbook)
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.